Skip to content
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 reinstall from cache cause optionalDependencies being deleted from package-lock.json #5282

Closed
wants to merge 0 commits into from

Conversation

Brooooooklyn
Copy link
Contributor

@Brooooooklyn Brooooooklyn requested a review from a team as a code owner August 9, 2022 10:25
@wraithgar wraithgar added the pr: needs tests requires tests before merging label Aug 9, 2022
@wraithgar wraithgar self-assigned this Aug 9, 2022
@wraithgar wraithgar removed the pr: needs tests requires tests before merging label Aug 9, 2022
@nlf
Copy link
Contributor

nlf commented Aug 10, 2022

unfortunately this isn't quite the right approach here, this changed behavior means that we'll always try to install an optional dependency, even if we're asked specifically not to. what we'll have to do is try to write a test that demonstrates the incorrect behavior, then work backwards and figure out why it's happening.

i'm working on a related problem with optional peer dependencies that may address this too, when that work is done i'll circle back here and see if anything changes

@Brooooooklyn
Copy link
Contributor Author

this changed behavior means that we'll always try to install an optional dependency, even if we're asked specifically not to.

That's the right behavior, right? The package-lock.json is designed to use cross-platform. Skip installation on one platform doesn't mean it needs to be skipped on the other platform.
Even if it happened on the same machine, I mean, delete package-lock.json and install again with node_modules existing, it should try to install the optionalDependencies again.
For example, if one of the optionalDependencies failed to install because lack of system components like gcc. Then the user configures the required system components and installs again, shouldn't try again in this scenario?

@wraithgar wraithgar removed their assignment Aug 11, 2022
@nlf
Copy link
Contributor

nlf commented Aug 26, 2022

this changed behavior means that we'll always try to install an optional dependency, even if we're asked specifically not to.

That's the right behavior, right?

if we're explicitly asked to not install a package, then no this is not the right behavior. consider npm i --omit=optional; with your changes here we would install optional dependencies despite having been instructed not to. i think you're on the right path, but we need to figure out a more specific approach so that we allow an optional node to fail installation, correctly omit it when asked and still record it in the package-lock.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants