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

Relax NPM dependency version conflict resolution #99

Closed
nigredo-tori opened this Issue Jan 23, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@nigredo-tori
Contributor

nigredo-tori commented Jan 23, 2017

Current implementation requires that for each NPM package all version descriptors match exactly (for example, 15.4.2 and ~15.4.2 don't match). So any such "conflict" must be resolved explicitly via npmResolutions. This is not a big problem at the moment, but is likely to become more of an issue as the number of libraries using the bundler grows.
There are two ways to deal with this I can think of:

  1. Automatic resolution for such conflicts within the plugin itself. This would, however, require adding and maintaining conflict resolution code compatible with package.json specification.
  2. A much simpler solution would be to delegate such resolution to npm/yarn themselves. package.json supports passing multiple version descriptors, which should be perfect for this purpose.
@julienrf

This comment has been minimized.

Show comment
Hide comment
@julienrf

julienrf Jan 23, 2017

Member

A much simpler solution would be to delegate such resolution to npm/yarn themselves.

That’s the best approach, in my opinion. I didn’t know we could pass multiple version descriptors in package.json, I just had a look at the spec and it seems we can indeed use || between version candidates, is this what you were thinking of?

Member

julienrf commented Jan 23, 2017

A much simpler solution would be to delegate such resolution to npm/yarn themselves.

That’s the best approach, in my opinion. I didn’t know we could pass multiple version descriptors in package.json, I just had a look at the spec and it seems we can indeed use || between version candidates, is this what you were thinking of?

@julienrf julienrf added this to the 1.0 milestone Jan 23, 2017

@nigredo-tori

This comment has been minimized.

Show comment
Hide comment
@nigredo-tori

nigredo-tori Jan 23, 2017

Contributor

@julienrf, || is disjunction. We need the opposite (conjunction), so that if any of the predicates don't match, the task fails. In package.json this just means adding the descriptors to space-separated list. E.g. ">=14.0.0 14.4.2 ^14.4.1".

Contributor

nigredo-tori commented Jan 23, 2017

@julienrf, || is disjunction. We need the opposite (conjunction), so that if any of the predicates don't match, the task fails. In package.json this just means adding the descriptors to space-separated list. E.g. ">=14.0.0 14.4.2 ^14.4.1".

@julienrf

This comment has been minimized.

Show comment
Hide comment
@julienrf

julienrf Jan 23, 2017

Member

Oops, you’re right, disjunction is not what we want… Thank for your explanation.

Member

julienrf commented Jan 23, 2017

Oops, you’re right, disjunction is not what we want… Thank for your explanation.

@julienrf

This comment has been minimized.

Show comment
Hide comment
@julienrf

julienrf Jan 23, 2017

Member

So, I think the way to go is to remove the npmResolutions key and to simply add all the versions of a same dependency in the package.json file, just as you explained.

Member

julienrf commented Jan 23, 2017

So, I think the way to go is to remove the npmResolutions key and to simply add all the versions of a same dependency in the package.json file, just as you explained.

@nigredo-tori

This comment has been minimized.

Show comment
Hide comment
@nigredo-tori

nigredo-tori Jan 23, 2017

Contributor

In my opinion, npmResolution should stay. We still need an escape hatch to manually resolve a conflict. What if someone adds an overly specific descriptor for a dependency in some package I depend upon? I'd like to be able to override this.

Contributor

nigredo-tori commented Jan 23, 2017

In my opinion, npmResolution should stay. We still need an escape hatch to manually resolve a conflict. What if someone adds an overly specific descriptor for a dependency in some package I depend upon? I'd like to be able to override this.

@julienrf

This comment has been minimized.

Show comment
Hide comment
@julienrf

julienrf Jan 23, 2017

Member

The current strategy is to use the npmResolutions only in the case where we have several versions of a same dependency. If we want to keep an escape hatch then the behavior should be the following when we have several versions of a same dependency: if there is an npmResolution for this dependency, use only this version, otherwise use all the versions (instead of failing, as we currently do). Does that sound good to you?

Member

julienrf commented Jan 23, 2017

The current strategy is to use the npmResolutions only in the case where we have several versions of a same dependency. If we want to keep an escape hatch then the behavior should be the following when we have several versions of a same dependency: if there is an npmResolution for this dependency, use only this version, otherwise use all the versions (instead of failing, as we currently do). Does that sound good to you?

@nigredo-tori

This comment has been minimized.

Show comment
Hide comment
@nigredo-tori

nigredo-tori Jan 23, 2017

Contributor

Yes, that's what I had in mind as well.

Contributor

nigredo-tori commented Jan 23, 2017

Yes, that's what I had in mind as well.

@julienrf julienrf modified the milestones: 0.6, 1.0 Feb 1, 2017

@julienrf julienrf modified the milestones: 0.7, 0.6 Apr 26, 2017

@julienrf julienrf modified the milestones: 0.8, 0.7 Jul 4, 2017

@julienrf julienrf modified the milestones: 0.8, 0.9 Sep 22, 2017

@julienrf julienrf modified the milestones: 0.9, 1.0 Oct 12, 2017

@julienrf julienrf closed this in #215 Feb 5, 2018

julienrf added a commit that referenced this issue Feb 5, 2018

Fix #99: Relax NPM dependency version conflict resolution (#215)
* feat: Delegate a version conflict resolution to npm/yarn if npmResolutions not provided.

* doc: Add a description regarding delegation to npm/yarn

exoego added a commit to exoego/scalajs-bundler that referenced this issue Feb 10, 2018

Fix #99: Relax NPM dependency version conflict resolution (#215)
* feat: Delegate a version conflict resolution to npm/yarn if npmResolutions not provided.

* doc: Add a description regarding delegation to npm/yarn

exoego added a commit to exoego/scalajs-bundler that referenced this issue Feb 10, 2018

Fix #99: Relax NPM dependency version conflict resolution (#215)
* feat: Delegate a version conflict resolution to npm/yarn if npmResolutions not provided.

* doc: Add a description regarding delegation to npm/yarn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment