This repository has been archived by the owner on May 3, 2024. It is now read-only.
feat(styleLoader): aggregate stylesheets and dedupe if already loaded #1099
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
|
Size Change: 0 B Total Size: 689 kB ℹ️ View Unchanged
|
JAdshead
reviewed
Aug 30, 2023
JAdshead
reviewed
Aug 30, 2023
JAdshead
reviewed
Aug 30, 2023
JAdshead
reviewed
Aug 30, 2023
JAdshead
reviewed
Aug 30, 2023
__tests__/server/utils/__snapshots__/renderModuleStyles.spec.js.snap
Outdated
Show resolved
Hide resolved
JAdshead
reviewed
Aug 30, 2023
JAdshead
suggested changes
Aug 30, 2023
…com/americanexpress/one-app into feature/aggregate-ssr-styles-loader
Thanks! and thanks for the feedback, addressed all comments |
Matthew-Mallimo
approved these changes
Aug 31, 2023
| @@ -1,7 +1,11 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|
|||
| exports[`renderModuleStyles should handle a mix of modules with and without SSR styles 1`] = `"<style class="ssr-css">.test-module-b_class { color: red; }</style><style class="ssr-css">.test-module-d_class { color: red; }</style>"`; | |||
| exports[`renderModuleStyles should deduplicate aggregatedStyles that have the same digest hash 1`] = `"<style id="shared_hash_deps" data-ssr="true">.test-module-a_deps { color: blue; }</style><style id="shared_hash_local" data-ssr="true">.test-module-a_local { color: rebeccapurple; }</style><style id="unique-hash_deps" data-ssr="true">.test-module-c_deps { color: blue; }</style><style id="unique-hash_local" data-ssr="true">.test-module-c_local { color: rebeccapurple; }</style>"`; | |||
There was a problem hiding this comment.
What is the difference between data-ssr="true' and class="ssr-css" ?
There was a problem hiding this comment.
strictly speaking none - although a style tag ought not have a class name because we're not applying styles onto the element. The idea here was more of a debug step to allow us to more easily track which styles are being hydrated and which are not. We could dispense with this entirely, but it felt like that might still be a useful step for more easily debugging / visualising the changes.
JAdshead
approved these changes
Aug 31, 2023
code-forger
approved these changes
Sep 1, 2023
zacowan
added a commit
that referenced
this pull request
Feb 7, 2024
Lift and shift of #1099 for v5.
14 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
This PR:
Motivation and Context
When using a component library (or any dependency) that imports styles via CSS Modules, the bundler output generates a server bundle that is inconsistent with the client side bundle, as well as resulting in it being difficult to provide style overrides without resorting to CSS hacks (like !important).
In order to resolve this and match the client side bundle output it's necessary to split the styles into multiple style tags, and then order them to allow for cascade overrides. However, the OneApp styles loader currently wraps all SSR styles in a single style tag.
This change retains the existing functionality in order to provide backwards compatibility for modules generated with older bundlers, whilst allowing for bundlers to provide a new key:
aggregatedStyles.How Has This Been Tested?
Generated a root module and three child modules that share a component library dependency, with each child module rendering two instances of the same component. The first one unchanged, the second with style overrides applied.
Multiple modules helped to prove that deduplication happened as intended, with only a single instance of the component library styles being rendered, with overrides applying with the expected cascade priority.
This was additionally run with an older version of OneApp, and an older bundle to prove backwards compatibility is unaffected.
Types of Changes
Checklist:
What is the Impact to Developers Using One App?
This allows developers to provide overrides to styles imported via CSS modules within their applications without resorting to CSS hacks and forcing specificity.