Move assets plugin registration to after app-level plugins

[angus-c]

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

[mlmorg]

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

[lhorie]

Registration order doesn't affect order in which plugins run. If you want to add the AssetsPlugin last, you need to modify the underlying array like reverseRegister does.

[ganemone]

Registration order absolutely does determine the order in which plugins are run. This code is correct as far as I can tell

[lhorie]

Isn't it going to get shuffled with the other ones once the depedency graph is resolved?

[angus-c]

It seems to work™

[ganemone]

Any two apps that don't depend on one another will be ordered respectively based on registration order. So this is what we want

[lhorie]

Ok sounds good. On a separate note, should it be app.register(AssetsPlugin)?

[ganemone]

oh yea it should be. This might be working technically because fusion would be treating app as a token lol. However I'm surprised this passes flow

[angus-c]

Yeah good catch

[rtsao]

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]

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]

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]

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]

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]

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]

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

[old-fusion-bot]

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