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

Remove temporary resolutions field once svgo fixes their issue #55

Closed
gomorizsolt opened this issue Oct 30, 2019 · 7 comments
Closed

Remove temporary resolutions field once svgo fixes their issue #55

gomorizsolt opened this issue Oct 30, 2019 · 7 comments

Comments

@gomorizsolt
Copy link
Collaborator

gomorizsolt commented Oct 30, 2019

See:

@gomorizsolt gomorizsolt changed the title Remove temporary resolutions field once svgo fixed the issue Remove temporary resolutions field once svgo fixes the issue Oct 30, 2019
@thisismydesign thisismydesign changed the title Remove temporary resolutions field once svgo fixes the issue Remove temporary resolutions field once svgo fixes their issue Oct 30, 2019
@thisismydesign
Copy link
Member

thisismydesign commented Jan 31, 2020

The issue seems to be resolved now svg/svgo#1176, released in https://github.com/svg/svgo/releases/tag/v1.3.2

@thisismydesign
Copy link
Member

There was some discussion and misunderstanding about this in #62.

TLDR; requirements:

  • specify a non-exact dependency (e.g. 1.3.2 and above)
  • compatibility of both npm and yarn if possible

See also: https://stackoverflow.com/questions/15806152/how-do-i-override-nested-npm-dependency-versions

Looks like npm has npm shrinkwrap for this, while yarn has resolutions and neither of them is compatible with the other manager. Let's just add svgo as a direct dependency with the correct version and a comment.

@thisismydesign
Copy link
Member

It's really weird but I can't seem to find a good description of this. My gut feeling is that the solution I proposed wouldn't work. AFAIU

if an alternate (but still "satisfying") dependency is already installed "further up the tree" as it were, then the buggy one will mercifully NOT get installed as a sub-submodule.

However, this doesn't limit the case when some dependency would explicitly require a broken sub-dependency. In this, we want to tell the user that this will not work. This is a nice free-time investigation if someone wants to score some SO karma, as none of the mentioned threads explain this properly.

Let's stick with yarn for now. Use ^1.3.2 resolution and add a comment to package.json with this issue as well as a mention to the readme to use yarn.

References:

@gomorizsolt
Copy link
Collaborator Author

As there's no permanent solution for this issue, should we leave this open?

@gomorizsolt
Copy link
Collaborator Author

Also, since there's an update in package.json, shouldn't we increment the version to v0.2.1?

@thisismydesign
Copy link
Member

thisismydesign commented Feb 7, 2020

Right, there should be a new patch release for this. Afterward, the issue can be closed. The solution we added is good and permanent, it's just that it only supports yarn.

@gomorizsolt
Copy link
Collaborator Author

gomorizsolt commented Feb 7, 2020

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

2 participants