Comments (7)
Thanks for the discussion around this; it's always good to justify the decisions made in software development.
From Koa's docs: koajs/koa@
master
/docs/guide.md#writing-middlewareWhen the middleware is run, it must manually invoke next() to run the "downstream" middleware.
Correct; if you want to run the downstream middleware then you must invoke next()
. We don't want to run the downstream middleware if we've already handled the request - we want to bypass it. From further down the same guide.md
page you linked (and previously linked in my comment explaining the decision: #2073 (comment) ):
Middleware that decide to respond to a request and wish to bypass downstream middleware may simply omit
next()
. Typically this will be in routing middleware, but this can be performed by any.
PostGraphile is effectively routing middleware; either it routes the request to PostGraphile's response handler, or it calls next()
to hand the unhandled request down to downstream middleware. We don't want users accidentally handing the request twice via downstream middleware, so we don't call next()
if we've already handled the request. This aligns with the guide.md
file that you have linked to, and does not limit the actions you can take since you can add middleware before PostGraphile to add before and after actions.
The middleware in this repo today, does not invoke
next()
which is not what most developers using middlewares expect.
I'm not convinced this is true, I think calling next()
after handling a request would be more surprising for most developers that are familiar with Koa v2+'s middleware system. We have many Koa users, and I don't recall this being raised before.
I'd usually expect a terminal middleware to be written by application owners, rather than a library we use.
Interesting take, and not one I am familiar with. Isn't it quite common for response middleware to be terminal middleware? What other libraries do you use that provide response middleware?
Just looking at some of the standard middleware the koajs project itself has authored:
- The
favicon
middleware only callsnext()
if the request was not for the favicon. - The
static
middleware only callsnext()
if sending the file failed - The
ratelimit
middleware doesn't callnext()
when it announces the limit is exceeded - The
jwt
middleware usesctx.throw
to throw an error, thereby not calling next().
However, if you are set on keeping PostGraphile this way, perhaps we can contribute to documentation so that others don't run into the same problem.
I'm still open to being convinced otherwise, but currently I believe that PostGraphile does the correct thing as outlined by the Koa documentation, and as demonstrated by a number of the Koa official modules.
A contribution to the documentation that indicates that non-terminal middleware (compression, authentication, authorization, cors, timing, logging, telemetry, body parsing, session, etc) must be added before PostGraphile would be welcome. This may be self-evident for many of these use cases since they need to "wrap" the request (in particular timing, telemetry, session, etc) or need to execute before the request (authentication, body parsing, cors, etc) but it does seem it warrants spelling out for middlewares that only need to execute after the response has been formed.
from crystal.
@benjie Thanks for the thorough and thoughtful response. Treating postgraphile as a terminal middleware makes sense to me. The node/koa ecosystem is new to me, so thank you for linking to example projects that do the same. I started looking through them myself. The way you have suggested handling telemetry, compression, etc, aligns with my model of middleware in Rails, as well. Thanks again.
from crystal.
You're welcome! I mostly did the research to ensure that we were doing the right thing, it's important to match peoples expectations and it's challenging to do when there's so many frameworks and I only use a couple of them. I am definitely not infallible!
The term "middleware" for a -ware that doesn't really go in the middle but at the end is a poor choice in terminology, so I definitely understand the confusion. But... that's just what they are called in Koa 🤷♂️ Really they're the only "-ware" it uses... They should call it "everyware" 😆
Contributions to the docs still very much welcome, especially since I don't use Koa myself - having people with hands on writing helpful docs for others would be really appreciated! The Koa docs in particular are extremely thin on the ground.
from crystal.
Closing for the reasons outlined in #2073 (comment)
The code can be fixed by moving the middleware before PostGraphile:
const app = new Koa();
// 👇👇👇
app.use(async (ctx, next) => {
console.log("running middleware BEFORE grafast");
await next(); // 👈👈 this is where PostGraphile/Grafast would run
console.log("running middleware AFTER grafast");
})
// 👆👆👆
const pgl = postgraphile(configuration, pool);
const serv = pgl.createServ(grafserv);
const server = app.listen(port);
await serv.addTo(app, server);
from crystal.
@benjie we have a requirement to run a middleware after, which was described in the issue I created as well. Middlewares in Koa should always be calling next(), which is why they can be chained.
What you are suggesting may work, but is not standard practice in middlewares for HTTP node servers.
What you suggest will work for us
from crystal.
@benjie Just to clarify, today postgraphile's middleware does not follow Koa guidelines, which my PR was trying to fix. This will be unexpected to anyone trying to use Koa + Postgraphile in the future.
I request to reconsider my changes.
From Koa's docs:
https://github.com/koajs/koa/blob/master/docs/guide.md#writing-middleware
When the middleware is run, it must manually invoke next() to run the "downstream" middleware.
The middleware in this repo today, does not invoke next()
which is not what most developers using middlewares expect.
from crystal.
@benjie I'm chiming in here because I work with @aryascripts. While your example in #2073 and your suggestion here is helpful and gives us something to work with, it was unexpected that PostGraphile would have a terminal middleware. I'd usually expect a terminal middleware to be written by application owners, rather than a library we use.
However, if you are set on keeping PostGraphile this way, perhaps we can contribute to documentation so that others don't run into the same problem.
from crystal.
Related Issues (20)
- Ruru rendering issues HOT 2
- GraphQL resolver emulation should cascade
- Should `__FlagStep` be isSyncAndSafe?
- Change how `__Flag` be represented in plan diagrams?
- Plan diagrams: `::sideeffectplan` and `::unbatchedPlan` conflict HOT 1
- RBAC enhancements: don't fetch column if not allowed to fetch column
- Rewrite OutputPlans, they're a mess
- `fieldArgs.getRaw` and related methods should throw if called out of turn
- Unable to pass FieldArg as parameter for resource .get() HOT 7
- Latest `grafast` depends on `graphile-config` but doesn't list dependency
- Foreign Key on Composite Type HOT 4
- PageInfo implemented incorrectly? HOT 6
- Hono adaptor HOT 2
- How to omit a class and specific field HOT 3
- pgUnionAll.single() throws an error when the list is empty HOT 2
- Grafast processFragment does not permit use of string arguments
- Use `@graphile/logger` for logging
- Improve pgSelectSingleFromRecord $record argument type
- How to Modify the Result of a PgSelectStep HOT 5
- How to get inflected database data in makeExtendSchemaPlugin? HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from crystal.