Navigation Menu

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

Please lock down your dependencies. #8180

Closed
sorensen opened this issue Aug 24, 2017 · 18 comments
Closed

Please lock down your dependencies. #8180

sorensen opened this issue Aug 24, 2017 · 18 comments

Comments

@sorensen
Copy link

As of the last few installs of this package of the same version, I have gotten different sub-modules and behavior. Primarily with validators.js, whatever module that may be.

Please, lock down your packages so that the rest of us don't all have different code running for the same versions of this module.

Sorry if this is coming off in a negative light, I am just fairly frustrated to have introduced changes into my projects without changing versions of this project.

@sorensen
Copy link
Author

sorensen commented Aug 24, 2017

To follow up on this, the primary issue is that the validators.js lib went from converting numbers to strings, and is now rejecting them with an error when given a number for a string field. This is an extreme behavior change.

@yonjah
Copy link
Contributor

yonjah commented Aug 24, 2017

I don't think it is up to Sequelize to lock down it's dependencies.
Packages that follow semver should follow semver and not introduce breaking changes in minor version changes.
Most users of sequelize will want to get the latest version of dependencies even if they are newer than what was originally was available when the version was released.

If as a user you want to lock down your dependencies after installing a dependency you should either move to npm5 or create a shrinkwrap

@yonjah
Copy link
Contributor

yonjah commented Aug 24, 2017

I see now that validator.js dependency was actually updated quite recently -
#8159
You probably did a minor version upgrade of sequelize without noticing and got the newer version

@sushantdhiman maybe validator.js should be kept at v6 until the next major release ?

@sorensen
Copy link
Author

The main issue I see here is that sequelize is using ^1.0.0 type versioning, which in itself leads to sub-modules being changed over time, even at the same version of sequelize. I absolutely lock down packages to specific versions, the issue here was upon re-installing sequelize at the same version, new code and behavior was introduced via submodules.

@sorensen
Copy link
Author

Some of this cant be avoided, if a module dependency is using a similar pattern for example, but this package could at the very least try to mitigate some of that by using exact module versions in its dependencies.

@yonjah
Copy link
Contributor

yonjah commented Aug 24, 2017

Some of this cant be avoided

It can be fully avoided if you use shrinkwrap (newer version of npm do it by default) -
https://docs.npmjs.com/files/package-locks

It is up to you as a user of packages to lockdown your dependencies.
Specifically with this issue nothing on Sequelize side can protect you from this if you don't lock down your dependencies. We can see from this bug you probably unintentionally installed a newer version of Sequlize without noticing (which upgraded to a new major version of validator.js intentionally).

If some Sequelize v5 is published today with v1.0.2 of module X and tomorrow v1.0.3 of version x is released including bugfixes and performance improvements without any breaking changes there is no reason for users installing Sequelize not getting the better performance of the newer version.

If Sequelize will lock it down all packages will be stale very quickly and a new version will need to be released each time a dependency is fixing it's bugs.

But if you do it yourself by utilizing NPM (as you should) you won't have this issues and you'll be free to upgrade when you decide to.

@sushantdhiman
Copy link
Contributor

sushantdhiman commented Aug 24, 2017

I expect any package to keep its dependencies up to date and but at same time don't introduce any breaking / unexpected changes.

Doing a major every time package try to update dependencies is not ideal either. So I try to update dependencies if we can patch them to have desired behavior. Most of these updates are minor (not patch).

When working on #8159 , I went through validator.js changelog. I just noticed isDate removal was one of breaking change, so I fixed it.

I am not sure what issue @sorensen faced, but if it was mentioned in changelog, I would have covered it.

But regardless if there is a breaking change, just let me know how to reproduce it. I will try to patch it, to conform to semver behavior.

@john-dodson
Copy link

@sushantdhiman It's easy to reproduce, try to insert an object of type Number into a sequelize string field. It will throw a validation error.

@sorensen
Copy link
Author

@yonjah We are using npm shrinkwrap, and have been using sequelize version 3.23.4 since June 30th 2016. This change was introduced regardless.

@sushantdhiman
Copy link
Contributor

Hmm, which version you updated from, what is your current version @sorensen

@sorensen
Copy link
Author

The version has not changed. Still using 3.23.4. A reinstall of the module introduced new dependencies.

@sushantdhiman
Copy link
Contributor

@sorensen I dont think npm allow republishing a already published version, neither I have changed any dependencies nor anything else in v3.

Are you sure validator.js was not updated by other dependencies. There is no new v3 version released in ages

@sorensen
Copy link
Author

Again, this comes back to locking down packages in the module. Any install of this module will produce different results over time because it is using ^1.0.0 type versioning for dependencies.

For a more extreme example, imagine if the package.json was all * versions.

@yonjah
Copy link
Contributor

yonjah commented Aug 24, 2017 via email

@sushantdhiman
Copy link
Contributor

sushantdhiman commented Aug 24, 2017

@sorensen Your npm and node version ?

@sorensen
Copy link
Author

@yonjah Agreed, it is, we had the misfortune of needing to re-roll the npm-shrinkwrap file and is what caused the new sub-module versions. This could have been solved by hand editing the shrinkwrap file itself, or in our case updating the breaking code.

This is not an issue that needs help fixing on my end, it is a complaint / suggestion to lock down module versions exactly and not use wildcard versioning, so that the issues described above don't continue to happen for others when re-rolling the shrinkwrap file, or for those who don't use one.

@sushantdhiman
Copy link
Contributor

As a package locking all dependencies to exact version is not practical as they tend to become outdated very soon.

I think package-lock.json and shrinkwrap.json are exact thing which are built for locking dependencies. Sequelize cant control your management of these lock files and as an user you will have to manage them properly.

@john-dodson
Copy link

Yeah, this isn't a package locking issue. You have a bug because validators.js has a bug. If someone starts a new project and uses sequelize, this will be an issue for them. Is your resolution for those people to version lock, then go edit the version to a previous version of validators.js that works with sequelize?

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