Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn if importing core, overrides without dependencies #1807

Merged
merged 1 commit into from
May 13, 2020

Conversation

36degrees
Copy link
Contributor

Add deprecation warnings if users import files from the core and overrides layers without first importing the settings, helpers and tools layers.

We'll remove the imports of "base" from files in these layers in 4.0, so this will no longer work.

Closes #1798

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1807 May 12, 2020 15:31 Inactive
@36degrees 36degrees force-pushed the warn-import-core-overrides-without-dependencies branch from db88ee4 to 6c79763 Compare May 12, 2020 15:32
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1807 May 12, 2020 15:32 Inactive
Add deprecation warnings if users import files from the core and overrides layers without first importing the settings, helpers and tools layers.

We'll remove the imports of "base" from files in these layers in 4.0[1], so this will no longer work.

[1]: #1800
@36degrees
Copy link
Contributor Author

@m-green it'd be good to get your thoughts on the deprecation notice we're using:

Importing items from the [core | overrides] layer without first importing base is deprecated, and will no longer work as of GOV.UK Frontend v4.0.

@36degrees 36degrees requested a review from m-green May 12, 2020 16:22
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that I don't see a warning as long as I import just one of /tools/all or /helpers/all (and no warning if I neglect to import /settings/all). This is an edge case but you wouldn't see a warning if you were already doing

@import "govuk-frontend/govuk/helpers/all";
@import "govuk-frontend/govuk/core/global-styles";

@36degrees
Copy link
Contributor Author

I think that's probably OK as an edge case, as it should also be communicated in the release notes (which should probably be done as part of this PR) and in the release notes for 4.0 when it goes out? Thoughts?

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @36degrees yes I think that makes sense, just wanted to flag it as something to be aware of.

Nice work with getting this done 👍

@36degrees 36degrees merged commit 387c33d into master May 13, 2020
@36degrees 36degrees deleted the warn-import-core-overrides-without-dependencies branch May 13, 2020 14:31
@36degrees 36degrees mentioned this pull request May 19, 2020
7 tasks
@36degrees 36degrees changed the title Warn if importing core, overrides w/o dependencies Warn if importing core, overrides without dependencies May 20, 2020
36degrees added a commit that referenced this pull request May 20, 2020
36degrees added a commit that referenced this pull request May 20, 2020
Add deprecation in #1807 to changelog, and add missing PR link for #1804
@36degrees 36degrees mentioned this pull request Jun 1, 2020
36degrees added a commit that referenced this pull request Dec 2, 2021
Importing files from this layer without first importing the 'base' file was deprecated in #1807. Anyone who was relying on the import should already be seeing warnings. If this change affects them, they'll just need to ensure they're importing the base layer first.

This is part of a change to the way that the base layers (settings, tools and helpers) are imported within different parts of GOV.UK Frontend’s Sass, designed to reduce the time it takes to compile the Sass to CSS.

Remove the tests, which were only checking that the deprecation warning was emitted as expected.
36degrees added a commit that referenced this pull request Dec 2, 2021
Importing files from this layer without first importing the 'base' file was deprecated in #1807. Anyone who was relying on the import should already be seeing warnings. If this change affects them, they'll just need to ensure they're importing the base layer first.

This is part of a change to the way that the base layers (settings, tools and helpers) are imported within different parts of GOV.UK Frontend’s Sass, designed to reduce the time it takes to compile the Sass to CSS.

Remove the tests, which were only checking that the deprecation warning was emitted as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate importing files from the core and overrides layers without importing dependencies
3 participants