Allow components to be imported without dependencies #1804
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
Add a new file called _index.scss to each component, which contains the component’s styles but does not import the settings, helpers and tools layers. Update the existing _[component].scss files to import the settings, helpers and tools layers and import _index.scss. This will preserve the existing behaviour, and continue to provide a single entry point for individual components that can be used in contexts such as React. We already have a test in src/govuk/components/all.test.js which ensures that the _[component].scss file renders without errors. We're doing this so that we can reduce the number of imports within the codebase – by moving everything except the imports into a separate file (`_index.scss`) and including that in the original _[component].scss file, we can then import just the _index.scss file when importing all components, shortcutting all of the base layer imports.
We already import `helpers/all`, so we don't need to then import the typography helpers separately.
This avoids importing the base layers again for every component. We need to explicitly import `accordion/index` file, rather than just importing `accordion`, as index file support has only been in LibSass since 3.6.0 and Ruby Sass since 3.6.0[1], and we support 3.3.0 and 1.0.0 respectively. Add an import of the base layers, so that this isn't a breaking change for anyone currently importing `components/all` without also importing the base layers. [1]: https://sass-lang.com/documentation/at-rules/import#index-files
Provide a single import for the settings, tools and helpers layers, which all of the other layers rely on.
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1804
RubySassBefore: ~26sAfter: ~16sLibSassBefore: ~0.20sAfter: ~0.18sDartSassBefore: 0.5sAfter: ~0.4s |
|
This is very similar to #1752, which was closed. For context:
|
|
There's a pre-release of this available for testing: |
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1804
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1804
hannalaakso
approved these changes
May 7, 2020
There was a problem hiding this comment.
This looks great @36degrees 👏 Just added a couple of small suggestions for the changelog. (Also, I don't know if there's anything we want to call out in the PR description that wasn't captured in the issue.)
CHANGELOG.md
Outdated
| If you currently import a subset of GOV.UK Frontend: | ||
|
|
||
| - you can now import `node-modules/govuk-frontend/govuk/base` instead of importing `node-modules/govuk-frontend/govuk/settings/all`, `node-modules/govuk-frontend/govuk/helpers/all` and `node-modules/govuk-frontend/govuk/tools/all` | ||
| - you should import components using their `index` file, which will avoid importing the dependencies again |
There was a problem hiding this comment.
Should we say that you can also just continue to import accordion/accordion (but won't get the improvements if you do)? In case someone doesn't want to do this right now.
CHANGELOG.md
Outdated
| If you currently import a subset of GOV.UK Frontend: | ||
|
|
||
| - you can now import `node-modules/govuk-frontend/govuk/base` instead of importing `node-modules/govuk-frontend/govuk/settings/all`, `node-modules/govuk-frontend/govuk/helpers/all` and `node-modules/govuk-frontend/govuk/tools/all` | ||
| - you should import components using their `index` file, which will avoid importing the dependencies again |
There was a problem hiding this comment.
Wondering if this should be the first bullet point on the list? It's the one where users will see the most benefit.
This avoids importing the base layers again when components import other components as dependencies. We need to explicitly import `accordion/index` file, rather than just importing `accordion`, as index file support has only been in LibSass since 3.6.0 and Ruby Sass since 3.6.0[1], and we support 3.3.0 and 1.0.0 respectively. [1]: https://sass-lang.com/documentation/at-rules/import#index-files
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1804
|
Benchmarks after fixing imports of dependent components in 3e471f8: RubySass: ~6.5sLibSass: ~0.16sDartSass: ~0.23s |
36degrees
added a commit
that referenced
this pull request
May 20, 2020
Merged
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.
What
Add a new file called
_index.scssto each component, which contains the component’s styles but does not import the settings, helpers and tools layers.Update the existing _[component].scss files to import the settings, helpers and tools layers and import _index.scss. This will preserve the existing behaviour, and continue to provide a single entry point for individual components that can be used in contexts such as React.
Update
components/_all.scssto include every component’s_index.scssfile, avoiding the unnecessary imports of the settings, helpers and tools layers within each component.Where components import other dependents, update to import their
_index.scssfile, avoiding the unnecessary imports of the settings, helpers and tools layers within the dependent components.Why
As proposed in alphagov/govuk-design-system-architecture#22, we’re making changes to the way that the base layers (settings, tools and helpers) are imported within different parts of GOV.UK Frontend’s Sass, to reduce the time it takes to compile to CSS.
Closes #1797