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

Consider locking down dependencies to patch versions only #1470

Closed
kevlarr opened this issue Jan 28, 2019 · 17 comments
Closed

Consider locking down dependencies to patch versions only #1470

kevlarr opened this issue Jan 28, 2019 · 17 comments

Comments

@kevlarr
Copy link

kevlarr commented Jan 28, 2019

Our project relies on Mirage locked to 0.4.0 which has a dependency of "ember-lodash": "^4.17.3"

My coworker built awhile ago, so npm install pulled in Mirage with ember-lodash version 4.18.0. Building today, npm install pulled the same version of Mirage but with ember-lodash version 4.19.4 instead, which itself pulled another dependency that is incompatible with Ember 2.11 (requiring 2.13+).

Using "^xx.yy.zz" versioning for all dependencies in package.json effectively means that patch versions of Mirage will update minor versions of dependencies, which is against expectation.

I propose that dependencies be versioned to allow flexibility at the patch level only, eg."~xx.yy.zz".

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 29, 2019

@kevlarr I would disagree with this suggestion. The root cause of your problem is that ember-lodash did not bump their major version when updating their Ember requirements. IMHO it would need to be fixed over there.

The other thing is that I would suggest that you use a lockfile (yarn or npm), in which case you would have been able to stay on the ember-lodash version that worked for you. Even now, with yarn resolutions you could manually overwrite the version requirement as a short term workaround.

Ultimately I would suggest updating Ember CLI though, you could even update it without updating Ember itself.

@kevlarr
Copy link
Author

kevlarr commented Jan 29, 2019

You're disagreeing on the basis that ember-lodash only updated their minor version when a dependency of theirs updated a major version (valid criticism). How is that different from patch versions of Mirage being able to pull in minor version updates of dependencies? It feels like we're both following the same logic here, so I think I'm missing something.

FWIW yes, you're absolutely right - we should be using lock files, and ember-lodash should not have only changed versions at the minor level.

@samselikoff
Copy link
Collaborator

Yeah. Tricky thing is, say Mirage bumps its version of chalk from 1.1.1 to 1.1.2. Semver is really just a convention, not a guarantee, so that could break the world. In this case, what is the appropriate response?

Users can always use resolutions to work around this. And in the meantime, I guess the best path forward is to communicate to the author that they broke on a non-major version?

@ef4
Copy link
Contributor

ef4 commented Jan 29, 2019

I agree with @Turbo87 and want to add an even stronger reason: it's a bad thing for a library (including an ember addon) to use stricter dependencies than absolutely necessary. I think most ember addons already use dependencies that are too strict, because both ^ and ~ probably cause many addon authors to rule out earlier versions of their dependencies that actually still work. It would be better to use > in a lot of cases.

The reason this matters is deduplication. Today's deduplication story in ember-cli is... weirdly not constantly on fire, given that it mostly works by accident. Going forward I expect to have much more precise control, which is going to bring to light all the duplication horrors that are hiding in people's apps.

Libraries should be very lenient while apps should be very strict. As in, apps definitely need to use a lock file. Nothing else can really save you from upstream bugs striking at random when you don't want to deal with them.

Stricter library deps are both ineffective (because bugs can strike even in semver patch releases), and actively harmful (because they result in greater dependency duplication).

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 29, 2019

It would be better to use > in a lot of cases.

I disagree on that part though. See request/request-promise#299 for reasons why that is not a good idea.

@kevlarr
Copy link
Author

kevlarr commented Jan 29, 2019

Great thoughts both of you, thanks for the perspective!

... weirdly not constantly on fire, given that it mostly works by accident

😆

@ef4
Copy link
Contributor

ef4 commented Jan 29, 2019

Sure, amend what I say from >1 to >1 <3.

@kevlarr
Copy link
Author

kevlarr commented Jan 29, 2019

@samselikoff

Semver is really just a convention, not a guarantee, so that could break the world. In this case, what is the appropriate response?

Ah okay, that's a great point (and I think was the essence of what @Turbo87 was telling me and I was failing to understand).

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 29, 2019

to >1 <3.

isn't that essentially the same as ^2 though? 😉

@samselikoff
Copy link
Collaborator

Libraries should be very lenient while apps should be very strict.

This is the part I always remember, and while it's neither consistently followed nor respected by tooling in the large, it seems like a good general approach.

Thanks @ef4 and @Turbo87 for chiming in & sharing your thougths.

@kevlarr – I fail to understand this stuff every other month when new issues crop up. It's an absolute nightmare as far as I'm concerned, and extremely prohibitive to the powerful abstractions that could exist in the ecosystem were it solved by the tooling layer. I'm not really sure about general best practice advice in the future because I often hear different advice from knowledgable people.

Sorry you ran into the bug but resolutions is probably your best short-term fix, barring the ability to go ahead and upgrade!

@ef4
Copy link
Contributor

ef4 commented Jan 29, 2019

I meant >=. I want to include the 1.x series too.

The microlanguage for semver checking is not something I trust myself to get right without testing:

> require('semver').satisfies("1.0.1", ">=1 <3")
true
> require('semver').satisfies("2.0.1", ">=1 <3")
true

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 29, 2019

we started doing something like https://github.com/simplabs/ember-simple-auth/blob/master/package.json#L40 but that is also causing other issues, so I'm not sure how useful that is in practice...

@kevlarr
Copy link
Author

kevlarr commented Jan 29, 2019

Libraries should be very lenient while apps should be very strict.

I like this, it's not something I had really thought through enough in the past but it does seem like a great general approach, after following this conversation.

@samselikoff
Copy link
Collaborator

but that is also causing other issues

@Turbo87 what other issues if you don't mind me asking?

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 29, 2019

@samselikoff when you have one dep having a subdep of ember-fetch: ^5 and you have e.g. simple-auth with a subdep also of ember-fetch: ^5 then everything will be fine and they will likely resolve to the same ember-fetch release.

now you update simple-auth to a newer release that declares ember-fetch: ^5 || ^6. you would think that yarn would be clever enough to realize that the old ember-fetch still works, but in reality, it's always trying to use the most recent version. this makes you end up with ember-fetch@5 and ember-fetch@6 both included in your project, which can have quite problematic consequences because they can overwrite each other's files the way they are currently bundled.

tools like https://github.com/atlassian/yarn-deduplicate might help in these cases, but it's not exactly obvious that you've even run into such a problem because right now nothing warns you about the duplicate dependency and in some cases having duplicate dependencies is not such a big problem (e.g. Node.js code).

@samselikoff
Copy link
Collaborator

@Turbo87 I see. Is ember-cli-dependency-lint our best guard against this for now?

Is this why ember-jquery has that treeForVendor code, because that's the only way to truly ensure there's no duplication?

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 29, 2019

Is ember-cli-dependency-lint our best guard against this for now?

yeah, that should at least warn you about the problem if you run into it

Is this why ember-jquery has that treeForVendor code, because that's the only way to truly ensure there's no duplication?

idk, sorry

@kevlarr kevlarr closed this as completed Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants