-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
fix(core): docusaurus CLI should detect the correct yarn version when suggesting upgrades #9006
fix(core): docusaurus CLI should detect the correct yarn version when suggesting upgrades #9006
Conversation
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
Correct yarn version detection by checking `yarnPath` value inside `.yarnrc.yml` file Add js-yaml in package.json
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.
That looks a bit complicated to me 🤪
What about:
import shell from 'shelljs';
async function getYarnVersion() {
const fallbackYarnVersion = 1;
const yarnVersionResult = shell.exec("yarn --version");
if (yarnVersionResult.code === 0) {
const majorVersion = parseInt(yarnVersionResult.stdout.split(".")[0]);
if ( typeof majorVersion === "number" && !Number.isNaN(majorVersion) ) {
return majorVersion;
}
}
if (await fs.pathExists(path.resolve('yarn.lock'))) {
return fallbackYarnVersion;
}
return undefined;
}
And then usage:
const getUpgradeCommand = async () => {
const yarnVersion = await getYarnVersion();
if (!yarnVersion) {
return `npm i ${siteDocusaurusPackagesForUpdate}`;
}
return yarnVersion === 1
? `yarn upgrade ${siteDocusaurusPackagesForUpdate}`
: `yarn up ${siteDocusaurusPackagesForUpdate}`;
};
Actually, my first idea was more close to this one🤣 If there are no security concerns, I fully agree with this improvement. |
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.
I made a new commit based on the review !
�Nullish coalescing operator still provides the fallback value Co-authored-by: Joshua Chen <[email protected]>
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.
LGTM thanks 👍
Pre-flight checklist
Motivation
I've checked PR #8908.
Great idea to check which Yarn verison is being used.
However, the presence of a
.yarnrc.yml
file alone is not a definitive indicator of Yarn 2+ being used, as this file can be used by Yarn 1.x as well.Check out the capture image below
This can be happened when you change Yarn 2+ -> Yarn 1
yarn set version 1
So, I think we should check
yarnPath
value (by usingjs-yaml
),to make sure what Yarn version is being used.
Test Plan
npm
npm run start
npm i
commandrm -rf node_modules
yarn start
npm i
commandrm -rf node_modules
Test links
Deploy preview: https://deploy-preview-9006--docusaurus-2.netlify.app/
Related issues/PRs
Related PR : #8908