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

Pin all dependencies #703

Merged
merged 2 commits into from
Oct 6, 2017
Merged

Pin all dependencies #703

merged 2 commits into from
Oct 6, 2017

Conversation

ms10398
Copy link
Contributor

@ms10398 ms10398 commented Oct 1, 2017

Addresses #684

Summary of changes:

I have pinned all node dependencies

@phawxby
Copy link
Contributor

phawxby commented Oct 2, 2017

Why would you do this over a yarn.lock or a package-lock.json? Genuinely curious. Admittedly it should probably have one or the other.

@bmuenzenmeyer
Copy link
Member

@phawxby

Appreciate the question - it's a good one.

Pattern Lab has always been a balancing act between approachability and power. One reason we still ship with Mustache as the default is how simple it is. This fact was relayed to me just today by a designer. Shipping with hawt new stuff is sometimes more to learn for people already harried by many other factors - and therefore creating a base experience is important.

  • yarn is another layer of complexity. one that does curry many benefits, yes.
  • npm 5 is required for a package-lock.json file - which I think we will get to eventually

Until support (which follows LTS) reaches the point where we can assume most users are on Node 8 (perhaps that's soon judging by https://github.com/nodejs/Release) I intentionally wanted to do the simplest thing that would make users a bit more safe (albeit potentially behind in deps)

@phawxby
Copy link
Contributor

phawxby commented Oct 4, 2017

So essentially it comes down to the users may not have Yarn and may not be running Node 8, version pinning allows for specific versioning with legacy support

@ms10398
Copy link
Contributor Author

ms10398 commented Oct 5, 2017

Do I need to change anything in my PR

@bmuenzenmeyer
Copy link
Member

bmuenzenmeyer commented Oct 5, 2017

@ms10398

Do I need to change anything in my PR

You are good - the build is breaking due to something unrelated which I will look into.

edit: this was breaking for precisely the benefits the semver range provides, in this case patternengine-node-mustache required 1.0.2 not 1.0.0

@bmuenzenmeyer bmuenzenmeyer merged commit 6e696a5 into pattern-lab:dev Oct 6, 2017
@bmuenzenmeyer bmuenzenmeyer added hacktoberfest 🌾 https://hacktoberfest.digitalocean.com staged for next release 🏁 labels Oct 6, 2017
@jfroffice
Copy link

I don't think this was the best way to fix dependencies.

You could have use npm shrinkwrap for this use case.

@bmuenzenmeyer
Copy link
Member

bmuenzenmeyer commented Oct 11, 2017

@jfroffice I think that's a fair criticism. Admittedly I know nothing about how shrinkwrap works. On the 3.X branch we will be landing soon, I will be implementing the package-lock.json concept anyways, which will likely bring this issue back into focus (and possibly re-add the carets?)

@jfroffice
Copy link

npm shrinkwrap generates a npm-shrinkwrap.json like package-lock.json.

it is used to freeze your dependencies on a working version.

So on, you can use npm update to increment your version using the semver notation and you don't have to touch the package.json. Except when you want to upgrade your dependencies.

(and possibly re-add the carets?)

hopefully

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest 🌾 https://hacktoberfest.digitalocean.com
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants