search

fusionjs / fusion-cli

Migrated to https://github.com/fusionjs/fusionjs
MIT License
140 stars 37 forks source link

Move assets plugin registration to after app-level plugins #742

Closed angus-c closed 5 years ago

angus-c commented 5 years ago

We want the response to be sent as early as possible so other middlewares can react to ctx.status

mlmorg commented 5 years ago

Can you include a description of the reasoning for this change? Also, can we add a regression test, if this is fixing a bug?

rtsao commented 5 years ago

I'm a bit confused by this, I think the asset plugin should go before any user/app plugins.

The disk asset serving should succeed/fail early on the downstream so that any user plugins can respond appropriately based on this (e.g S3 fallback, custom headers, etc.)

ganemone commented 5 years ago

The disk asset serving should succeed/fail early on the downstream so that any user plugins can respond appropriately based on this (e.g S3 fallback, custom headers, etc.)

The reason we don't register this first is so the user can have middleware that run before. This allows the user to compose with the assets middleware to do things like add headers.

rtsao commented 5 years ago

The reason we don't register this first is so the user can have middleware that run before. This allows the user to compose with the assets middleware to do things like add headers.

The asset plugin doesn't await next?

ganemone commented 5 years ago

The reason we don't register this first is so the user can have middleware that run before. This allows the user to compose with the assets middleware to do things like add headers.

The asset plugin doesn't await next?

It does, and that is exactly why it needs to be registered last. Otherwise it will be the last thing to run, and we wont be able to have fallback mechanisms (such as s3).

rtsao commented 5 years ago

It does, and that is exactly why it needs to be registered last. Otherwise it will be the last thing to run, and we wont be able to have fallback mechanisms (such as s3).

The asset plugin should set the response body on the downstream though, right?

angus-c commented 5 years ago

It does, and that is exactly why it needs to be registered last. Otherwise it will be the last thing to run, and we wont be able to have fallback mechanisms (such as s3).

The asset plugin should set the response body on the downstream though, right?

Only if opts.defer is false. As of several months ago its been true, which I think means it is set on the upstream See: https://github.com/koajs/static/blob/master/index.js

angus-c commented 5 years ago

test added. @rtsao do you still have qualms? See my opts.defer true/false comment above.

old-fusion-bot[bot] commented 5 years ago

Triggered Fusion.js build verification: https://buildkite.com/uberopensource/fusion-release-verification/builds/1772