Ensure Dart Sass file:// sources use relative paths
#3527
Merged
Conversation
This file contains hidden or 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
colinrotherham
added
🐛 bug
Contributor
Author
|
The GOV.UK Design System website also uses Dart Sass but isn't built locally:
|
colinrotherham
force-pushed
the
source-map-sources
branch
from
357271c to
945b204
Compare
colinrotherham
commented
Apr 20, 2023
|
|
||
| // Files to write | ||
| /** @type {SourceMap | undefined} */ | ||
| const map = result.map ? JSON.parse(result.map.toString()) : undefined |
Contributor
Author
There was a problem hiding this comment.
We need map.sources a few lines down so parse to SourceMap here
Otherwise across Terser, Rollup and PostCSS we see source maps as:
- JavaScript
objecttypes that needJSON.stringify() - Class instances with
.toJSON()and.toString()methods - Preprepared
stringtype - Missing
undefinedtype
This can be seen via the AssetOutput @typedef in this file
colinrotherham
commented
Apr 20, 2023
| * Make source file:// paths relative (e.g. for Dart Sass) | ||
| * {@link https://sass-lang.com/documentation/js-api/interfaces/CompileResult#sourceMap} | ||
| */ | ||
| .map((path) => path.startsWith('file://') |
Contributor
Author
There was a problem hiding this comment.
PostCSS adds the Dart Sass entry point ../src/all.scss as a relative path
But all the other Dart Sass sources are absolute path file:// URLs
colinrotherham
commented
Apr 20, 2023
| // Add Autoprefixer prefixes to all source '*.scss' files | ||
| .flatMap(mapPathTo(['**/*.scss'], ({ dir: requirePath, name }) => [ | ||
| join(requirePath, `${name}.scss`), | ||
| join(requirePath, `${name}.scss.map`) // with source map |
Contributor
Author
There was a problem hiding this comment.
Service teams with source maps enabled will have been inspecting our Autoprefixer-transformed sources, not the original prefix-free *.scss source files
colinrotherham
commented
Apr 20, 2023
| inline: false, | ||
| prev: map | ||
| if (typeof options.map === 'object') { | ||
| options.map.prev = map |
Contributor
Author
There was a problem hiding this comment.
PostCSS will automatically discover source maps via sourceMappingURL comments but we prefer to pass them directly from Dart Sass here (like we did previously)
We transform Sass sources (Autoprefixer only) but didn’t source map the changes
Using types from Terser for the Source Map Revision 3 spec
This is consistent with our Node Sass output
colinrotherham
force-pushed
the
source-map-sources
branch
from
945b204 to
2b7dd08
Compare
romaricpascal
approved these changes
Apr 21, 2023
Member
romaricpascal
left a comment
romaricpascal
left a comment
There was a problem hiding this comment.
I think that did it and it's all very localised! Thanks for adding tests as well 🙌🏻
colinrotherham
added a commit
to nhsuk/nhsuk-frontend
that referenced
this pull request
Feb 20, 2025
colinrotherham
added a commit
to nhsuk/nhsuk-frontend
that referenced
this pull request
Feb 20, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 21, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 24, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 24, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 24, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 25, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 25, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 25, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 26, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 26, 2025
colinrotherham
added a commit
to ministryofjustice/moj-frontend
that referenced
this pull request
Feb 26, 2025
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.
In preparation for v4.6.0 release we noticed Dart Sass writes absolute file paths to source map sources:
You can see we last released source maps (via Node Sass) with relative paths:
dist/govuk-frontend-4.5.0.min.css.map
dist/govuk-frontend-4.5.0.min.js.map
But when building with Dart Sass we see absolute paths to
govuk-frontendincluded:Node Sass source maps
{ "sources": [ "components/accordion/_index.scss" "components/back-link/_index.scss", "components/breadcrumbs/_index.scss" ] }Dart Sass source maps
{ "sources": [ "file:///Users/username/path/to/govuk-frontend/src/govuk/components/accordion/_index.scss" "file:///Users/username/path/to/govuk-frontend/src/govuk/components/back-link/_index.scss", "file:///Users/username/path/to/govuk-frontend/src/govuk/components/breadcrumbs/_index.scss" ] }This PR ensures we keep relative sources paths as we did with Node Sass