-
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
fix(install): Rebuild native modules when node version changes #4750
Conversation
This change will increase the build size from 9.94 MB to 9.94 MB, an increase of 1.26 KB (0%)
|
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.
Thanks! Two small nits and we're good to go 👍
src/integrity-checker.js
Outdated
@@ -37,6 +38,7 @@ type IntegrityHashLocation = { | |||
}; | |||
|
|||
type IntegrityFile = { | |||
node_version: string, |
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.
nodeVersion
(camelcase :)
src/integrity-checker.js
Outdated
@@ -386,6 +393,7 @@ export default class InstallationIntegrityChecker { | |||
integrityMatches: integrityMatches === 'OK', | |||
integrityError: integrityMatches === 'OK' ? undefined : integrityMatches, | |||
missingPatterns, | |||
nodeVersionDoesntMatch: integrityMatches === 'NODE_VERSION_DOESNT_MATCH', |
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.
Can you rename this to something more generic, like hardRefreshRequired
?
@arcanis sure and done. Thanks for reviewing 👍 😊 |
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.
Shall we exclude minor or patch versions from the refresh?
Maybe I'm something obvious, but I don't believe this solves my use-case for #756 -- build and package on one architecture, deploy and rebuild on another. I'm not worried about NODE_VERSION changing. "Bad idea," sure, but npm handles this well. |
…kg#4750) **Summary** Fixes yarnpkg#756. We have multiple versions of our app and each one uses a different version of node. Therefore we need to rebuild our `node-sass` module every time we move from one to another. This PR addresses that by saving the NODE version those artifacts were built with within the `.yarn-integrity` file and triggers forced scripts install (only if the node version is different ofc). **Test plan** ``` 1. Install Node.js 7.x 2. Add the node-sass dependency to the project via Yarn 3. Update Node.js to 8.x (new NODE_VERSION) 4. Run "yarn install" (you should see yarn downloading fresh scripts/binaries) ```
@aahoughton what if Yarn stored the arch. as well and rebuild upon changes? Would that solve the issue you're having? Anyway, for now I'd wait for the next release and create a separate issue describing the different architectures problem (if that's still a thing) - feel free to ping me then and I'll give it a go. Any thoughts on this ^ anyone - i.e. is that the right solution? |
That would solve my issue, yes -- I'd just need to insure that the appropriate files are built into my deb. This is effectively the same logic as the version rebuild trigger, yes? |
poke. |
Summary
Fixes #756. We have multiple versions of our app and each one uses a different version of node.
Therefore we need to rebuild our
node-sass
module every time we move from one to another.This PR addresses that by saving the NODE version those artifacts were built with within the
.yarn-integrity
file and triggers forced scripts install (only if the node version is different ofc).Test plan