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

Unpin dependencies. #719

Closed
wants to merge 1 commit into from
Closed

Unpin dependencies. #719

wants to merge 1 commit into from

Conversation

sholladay
Copy link
Contributor

@sholladay sholladay commented Feb 10, 2017

Intern has previously had a very inconsistent use of pinned and unpinned dependencies. This PR aims to normalize that and improve the user experience when upstream features are released that do not require code changes to Intern (e.g. new tunnels or env vars added to DigDug).

The basic premise here is that whatever gain is had from pinning the immediate dependencies is cancelled out by virtue of the nested dependencies using ranges themselves (extremely common, in part due to npm defaults).

So in effect, prior to this PR, Intern still experiences the dangers of version ranges, but without the benefits. I think Intern should either shrinkwrap its dependencies or use this PR.

I left dojo alone since it is pinned to a pre-release version. Using ^2.0.0-alpha.7 is valid, but since it matches both pre-releases and releases, if a breaking change happens prior to a proper release, that would be problematic, which seems within the realm of possibility (and is allowed by the spec).

Pinned dependencies give a false sense of security. Only shrinkwrapping can protect against semver abuse and other updating perils. In lieu of that, caret ranges have the best risk/reward story.
@jason0x43
Copy link
Member

I would say that Intern takes a reasonably consistent approach to its use of pinned dependencies; originally all dependencies were pinned, and currently all non-Intern dependencies are pinned. (Dig Dug and Leadfoot were unpinned so that bug fixes could come through without requiring a new Intern release.) I agree that only pinning top-level dependencies isn't particularly thorough, but while unpinning them may provide quicker access to new features, it magnifies the likelihood that something will break (as the CI test run shows).

At the moment I lean towards shrinkwrap, or at most using '~'.

@vladikoff
Copy link
Member

At the moment I lean towards shrinkwrap, or at most using '~'.

👍

@sholladay
Copy link
Contributor Author

all non-Intern dependencies are pinned

Fair. It is consistent in that sense. I would ask, though, if you trust updates from yourself, why not new features?

it magnifies the likelihood that something will break (as the CI test run shows).

I haven't looked deep into the root cause of the Travis failure, but it failed only on Node 0.12 with a simple AssertionError, indicating that there may be a bug. This doesn't look like semver abuse or anything like that. And again, yes, new bugs can happen occasionally with or without this PR. I would argue that this PR makes it more obvious why that can happen. It embraces a much more common convention. Another way to look at this is, which do you think is more common: new bugs OR (bug fixes AND new features)? Remember that we are talking about production releases here that have passed their respective test suites and CI runs, as all of Intern's immediate dependencies are high-quality projects that have these.

In regards to stability, note that many of your fellow testing frameworks such as AVA and Mocha use caret ranges and don't shrinkwrap. While I wouldn't use that as a reason alone to do this, I think it's meaningful to demonstrate that other stability-minded folks are getting along just fine with this.

@jason0x43
Copy link
Member

jason0x43 commented Feb 10, 2017

This doesn't look like semver abuse or anything like that. And again, yes, new bugs can happen occasionally with or without this PR. I would argue that this PR makes it more obvious why that can happen.

I'm not so concerned with why something is broken so much as the fact that it's broken. With pinned dependencies, we (theoretically) know that users will end up with a product that has been tested. When we use tilde ranges, the odds go up that a user will encounter something broken, but those odds should still be reasonably low since tilde implies small changes. With caret ranges the odds of encountering breaks go up again, because much larger changes are allowed.

Noticing that Intern is broken isn't too bad -- we can just run a daily or weekly CI cron job to check that current installs work. However, dealing with the problem becomes much more of an issue, because now all new installs of Intern are broken until we do something about it (either pinning to a known good version of a dependency or writing around the issue).

Another way to look at this is, which do you think is more common: new bugs OR (bug fixes AND new features)? Remember that we are talking about production releases here that have passed their respective test suites and CI runs, as all of Intern's immediate dependencies are high-quality projects that have these.

My main concern isn't that the projects aren't tested, it's that they aren't necessarily tested in the same environments that Intern is. The CI failure is a good example of this. At least one of the dependencies wasn't tested in, or may explicitly no longer support, Node 0.12, which Intern still technically does.

In regards to stability, note that many of your fellow testing frameworks such as AVA and Mocha use caret ranges and don't shrinkwrap. While I wouldn't use that as a reason alone to do this, I think it's meaningful to demonstrate that other stability-minded folks are getting along just fine with this.

I'm not completely against using looser dependency versions, but I'm still not sure that the benefits necessarily outweigh the accompanying maintenance headaches.

@sholladay
Copy link
Contributor Author

Closing for lack of interest combined with merge conflicts and CI failure. 👻

@sholladay sholladay closed this May 8, 2017
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

Successfully merging this pull request may close these issues.

3 participants