Append styles imported in JS to end of document.head
#1045
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This addresses the issue raised in embroider-build#1033 ("2. Import app.scss from app.js") that link tags added through imports of CSS in JS get added next to the script entrypoint (at the body's end) rather than to the document's head, causing a FOUC in FastBoot rendered pages.
simonihmig
commented
Dec 10, 2021
| this.appendToHead(newTag); | ||
| } else { | ||
| // Keep the new style in the same place as the original one | ||
| this.insert(newTag); |
There was a problem hiding this comment.
This should ensure that styles already present in index.html will be kept where they are, to keep the existing order. Especially when dealing with external styles, that are not funneled through this. (e.g. vendor.css, then external google-font-css, then app.css). All other dynamically added styles will be appended to <head>, in whatever order webpack gives us.
This is all we can do here about ensuring the "right" order, is it?
|
The one failing CI job is unrelated (some network error w/ yarn) |
|
Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses the issue raised in #1033 ("2. Import app.scss from app.js") that link tags added through imports of CSS in JS get added next to the script entrypoint (at the body's end) rather than to the document's head, causing a FOUC in FastBoot rendered pages.
This was tested locally with my test app (https://github.com/simonihmig/embroider-sass-app/tree/import-app), so I am quite confident it does the right thing. Idk if we need automated tests for this, I haven't found any related test coverage, but maybe I missed something with the (new) test setup? We could add unit tests for the
Placeholderclass, but that seems to yield relatively little value given the rather trivial implementation. Better maybe would be some webpack-related e2e-like tests. To ensure the output in the generated index.html, especially the order of styles and scripts, one would probably need (jest) tests that parse the build output, rather than regular ember app tests?