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

Update favicons dependency to v5.3.0 #46

Merged
merged 1 commit into from
Feb 18, 2019
Merged

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Feb 1, 2019

... and also change it back to ^ constraint to take advantage of bugfixes and new features

@Turbo87
Copy link
Contributor Author

Turbo87 commented Feb 15, 2019

@davewasmer @Exelord any thoughts on this? this PR will be needed to actually be able to use the appShortName option

@Exelord
Copy link
Collaborator

Exelord commented Feb 16, 2019

I'm not a fan of ^ in dependencies as we talk earlier. The packages can introduce bugs without any notice with every version and we will populate that to every production apps. It's like setting a project with ember version of ^3.0. Theoritcaly everything should be backword compatible but we know that bugs happen sometimes.

Are you sure we want to follow this pattern here?

@Turbo87
Copy link
Contributor Author

Turbo87 commented Feb 18, 2019

the issues you're describing are solved by using lockfiles, that lock all dependencies to specific versions, instead of just the top-level deps.

Are you sure we want to follow this pattern here?

I am pretty sure about it, since I use it in literally all other projects too. I can change it in this PR if you don't agree, but I really see no point in pinning it to specific versions unless there actually is a known compat issue with an upstream dependency.

One example was QUnit, which dropped support for Node 4 without increasing their major version. Arguably they should have done that to comply with semver, but they didn't. ember-qunit always used a ^ constraint on the qunit dependency, but as soon as we were aware of their change we pinned the version to the latest compatible version, removed the ^ and then the problem was solved.

@Exelord Exelord merged commit b93c970 into davewasmer:master Feb 18, 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

Successfully merging this pull request may close these issues.

2 participants