-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
chore: upgrade to Yarn v4.0.x #6520
Conversation
@@ -53,5 +50,5 @@ function fixSourcePathsInSourceMap({ outputMapFile, rawSourceMap }) { | |||
const pathToSourceWithoutProtocol = source.replace("file://", ""); | |||
return relative(outputDirectory, pathToSourceWithoutProtocol); | |||
}); | |||
return new SourceMapGenerator(rawSourceMap).toString(); | |||
return SourceMapGenerator.fromSourceMap(new SourceMapConsumer(rawSourceMap)).toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like we were doing this wrong, because I was getting empty source maps. this new approach seems to work better.
docs: https://github.com/7rulnik/source-map-js#generating-a-source-map
@@ -11,10 +11,10 @@ | |||
}, | |||
"bin": { | |||
"assert-package-layout": "./assert-package-layout.mjs", | |||
"css-dist": "./css-dist.sh", | |||
"css-dist": "./css-dist.mjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the latest Yarn doesn't like running NPM binaries as shell scripts, it tries to run everything in Node.js. the easiest thing to do here is to migrate to JS scripts, even though it's a little more verbose. this also has the benefit of standardizing all the script formats, but it is a small breaking change.
@@ -217,7 +212,6 @@ jobs: | |||
- run: ./scripts/publish-npm-semver-tagged | |||
|
|||
workflows: | |||
version: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the CircleCI VSCode plugin told me that this key was deprecated / no longer necessary
- run: | ||
name: Check if yarn.lock changed during install | ||
command: git diff --exit-code | ||
- run: yarn install --immutable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that --immutable
does the git diff check for us and fails if the lockfile changes during install 👍🏽
@@ -46,7 +47,6 @@ | |||
"@types/sinon": "~17.0.0", | |||
"@types/yargs": "~17.0.22", | |||
"chai": "^4.3.7", | |||
"cross-env": "^7.0.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we no longer need cross-env
with Yarn v4; this is now built-in to the script runner
fix css-dist and sass-compile source mapsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
CSS source maps are working, but they only point to the CSS file output by the Sass compiler. We're not getting Sass source maps in production or dev builds right now. Looking into it. |
Attempt to fix sass source mapsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Changes proposed in this pull request:
Upgrade to Yarn v4 while keeping the same strategy for linking node modules.
We can migrate to a different install mode in a separate PR.
node-build-scripts:⚠️ break: migrate
css-dist
andsass-compile
from shell to Node.js scripts to improve compatibility with latest Yarn package managerReviewers should focus on:
All CI jobs still run properly
Screenshot
N/A