This repository was archived by the owner on Aug 11, 2022. It is now read-only.
Repository navigation
Conversation
iarna
force-pushed
the
iarna/shrinkwrap-opt-not-installable
branch
4 times, most recently
from
August 17, 2016 21:53
3a385ea to
2892ea6
Compare
| } | ||
| } | ||
| function thenRunAction () { | ||
| pkg.knownInstallable = true |
Contributor
There was a problem hiding this comment.
Why do you need andMarkInstallable at all here? It looks like both code paths hit thenRunAction invariably. Alternately, why is this in thenRunAction? In the path that skips andMarkInstallable, pkg.knownInstallable is already true.
Contributor
Author
There was a problem hiding this comment.
This is a good question…
Contributor
Author
There was a problem hiding this comment.
The answer is: This wasn't supposed to still be in thenRunAction
Contributor
|
LGTM! Couple of nits you're free to ignore, otherwise 🐑 🎉 |
iarna
force-pushed
the
iarna/shrinkwrap-opt-not-installable
branch
from
August 30, 2016 22:58
fb872ed to
60100c9
Compare
We were doing a weird thing where we used a package.json field _installable_ to check to see if we'd checked for platform compatibility, and if not did so. But this was the only place that was ever done so there was no reason to implement it in such an obfuscated manner. Instead it now just directly checks and then records that its done so on the node object with `knownInstallable`. This is useful to know because modules expanded via shrinkwrap don't go through this– `inflateShrinkwrap` does not currently have any rollback semantics and so checking this sort of thing there is unhelpful. PR-URL: #13692 Credit: @iarna Reviewed-By: @zkat
iarna
force-pushed
the
iarna/shrinkwrap-opt-not-installable
branch
from
August 30, 2016 23:00
60100c9 to
1a696cb
Compare
Modules that came into the tree via shrinkwrap won't have had this determined in advance so we check here and take advantage of the optional rollback semantics to skip them. Plus if it was a regular dependency we can just fail out now instead of trying and failing to install the dep. Fixes: #13394 PR-URL: #13692 Credit: @iarna Reviewed-By: @zkat
iarna
force-pushed
the
iarna/shrinkwrap-opt-not-installable
branch
from
August 30, 2016 23:58
1a696cb to
97b81de
Compare
|
Changes Unknown when pulling 97b81de on iarna/shrinkwrap-opt-not-installable into * on release-next*. |
Contributor
Author
|
Landed in |
zkat
pushed a commit
that referenced
this pull request
Sep 8, 2016
We were doing a weird thing where we used a package.json field _installable_ to check to see if we'd checked for platform compatibility, and if not did so. But this was the only place that was ever done so there was no reason to implement it in such an obfuscated manner. Instead it now just directly checks and then records that its done so on the node object with `knownInstallable`. This is useful to know because modules expanded via shrinkwrap don't go through this– `inflateShrinkwrap` does not currently have any rollback semantics and so checking this sort of thing there is unhelpful. PR-URL: #13692 Credit: @iarna Reviewed-By: @zkat
zkat
pushed a commit
that referenced
this pull request
Sep 8, 2016
Modules that came into the tree via shrinkwrap won't have had this determined in advance so we check here and take advantage of the optional rollback semantics to skip them. Plus if it was a regular dependency we can just fail out now instead of trying and failing to install the dep. Fixes: #13394 PR-URL: #13692 Credit: @iarna Reviewed-By: @zkat
4 tasks done
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The commits here will be rolled up a bit before merge.
Ordinarily, if an optional dep fails the arch/cpu checks then we never try to extract it or install it.
But if an optional dep is in an
npm-shrinkwrap.jsonfile then it would get injected into the tree without checking instability–this fixes that.In a perfect world we would do this step in
inflate-shrinkwrap. Adding that there will be somewhat involved however as it currently has no facility for skipping a dependency. We could just skip the opt dep that can't work but what we need it to do is ALSO skip any of its transitive deps as well. Worse, it might be a non-optional transitive dep of an optional dep that failed and we have to go back and remove the entire tree associated with the optional dep. Fortunately this is already implemented elsewhere…So instead I've implemented this by doing the check prior to running any actions. The action running stage already has facilities to deal with failing optional deps and by hooking this as a prerequisite to ANY actions, it means it will happen before the very first one, which is
fetch. This means performance-wise it should be roughly equivalent to doing this ininflate-shrinkwrapand it saves us from duplicating the optional-dep-failing code.