Sort addon css by package name #1866
Conversation
raycohen
changed the title
| } | ||
| } else { | ||
| result.push(resolve.sync(mod, options)); | ||
| } | ||
| } | ||
| if (styles.length) { | ||
| result = [...styles, ...result]; |
There was a problem hiding this comment.
this was unshifting the group of styles from each package to the overall list, which reverses the order and we no longer want. then in implicitScriptsAsset the entire list was getting (re-)reversed. We don't need that reverse either now.
|
Just a quick note on how to make sure this continues to work for the upcoming major release: @ef4 is correct that you should target stable with a change like this so that it could be released sooner than the next major, but we can't ensure that this is going to keep working unless we make sure to add a test that verifies the output is in the correct order. If this gets merged to stable and we then forward-port the test to the main branch we can ensure that the sort ordering is still correct even though the implementation is completely different 👍 |
|
I will look into testing this. Right now we're getting by with patch-package (with a slightly different diff) but we'll want to get off that eventually |
raycohen
force-pushed
the
sort-addon-css-alphabetically
branch
from
e96ddd1 to
fc820d1
Compare
| project.addDependency(addonA); | ||
| project.addDependency(addonB); | ||
| addonA.addDependency(addonC); | ||
| project.addDependency(addonD); |
There was a problem hiding this comment.
this test without the logic change gave c->a->b->d ordering
raycohen
changed the title
|
|
||
| module.exports = function (defaults) { | ||
| let app = new EmberApp(defaults, {}); | ||
| app.import('node_modules/third-party/third-party.css'); |
There was a problem hiding this comment.
add more of these so we get coverage on the order between app.import calls as well
|
isn't order of assets supposed to be dictated by addon order before/after options that the legacy pipeline controls with alphabetical being the starting point only? |
|
I'm not sure what the CI failures are here... |
In a classic build, I believe the answer is no. The vendor css concatenation doesn't consider any ordering info for Still not sure why this PR is red but I would like to get it landed! |
Fixes #1862
While broccoli-concat doesn't guarantee order, the effective order of addon css in (most?) classic builds seems to be alphabetical by package name.
Also, the order of
app.import'd styles was being reversed from what the classic build had, in addition to not coming before addon css.Updates:
stableimplicit-styles