-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Correctly handle installationMethod when using bundled build #3113
Conversation
src/util/yarn-version.js
Outdated
// Homebrew (so things like update notifications can point out the correct | ||
// command to upgrade). | ||
const manifestPath = path.join(__dirname, '..', 'package.json'); | ||
if (fs.existsSync(manifestPath)) { |
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.
why sync?
We use async/await in all the other places, don't we?
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 async version of existsSync
is deprecated.
cli/index.js also uses existsSync
and readFileSync
:
Lines 296 to 297 in 45af656
const possibleLoc = path.join(config.cwd, registries[registryName].filename); | |
const manifest = fs.existsSync(possibleLoc) ? fs.readFileSync(possibleLoc, 'utf8') : 'No manifest'; |
Having said that, I can change reading the file to be async by creating a getInstallationMethod
async function and using that where needed.
src/cli/index.js
Outdated
@@ -171,7 +171,7 @@ if (commander.json) { | |||
outputWrapper = false; | |||
} | |||
if (outputWrapper) { | |||
reporter.header(commandName, pkg); | |||
reporter.header(commandName, {name: 'Yarn', version}); |
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 think this line broke a test
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.
Oops - I thought I ran the tests locally, but I might have only run the linter :/ I'll fix it.
Hey @Daniel15, this needs a rebase |
Sure, I'll do that soon :) |
…o "yarn" so tests pass, and use proper Flow type for installation method.
Rebased, I'll merge once I verify that the tests pass. |
Summary
We do
require('package.json')
in a few places to get the version number and installation method of Yarn. In the Webpack build, this is embedded within the JS file itself. While this works fine for the version number (as that's constant), the installation methods can be different even with an identical build of Yarn. For example, Homebrew takes our tarball and then patches theinstallationMethod
inpackage.json
so we can tell that Yarn was installed via Homebrew. Usingrequire('package.json')
in this case won't reflect the update as it's only seeing package.json at the time of build (rather than at runtime).Instead, we need to actually read the
package.json
file to check for the installationMethod at runtime. This looks in the parent directory (../package.json
) due to the directory structure:Test plan
.\bin\yarn --version
, ensure it says0.24.0-0
.\bin\yarn install
. ensure heading says "Yarn install v0.24.0-0".\bin\yarn versions
, ensure.\scripts\build-webpack.js
then do all the same tests with.\artifacts\yarn-0.24.0.js
Closes #3085