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.

Sign up for GitHub

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

Jump to bottom

chore: install lz-string from git repo #932

Closed
wants to merge 2 commits into from
Closed

chore: install lz-string from git repo #932

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9db27b3
chore: install lz-string from git repo
Apr 19, 2021
651a12c
chore: install lz-string of latest commit hash
Apr 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"aria-query": "^4.2.2",
"chalk": "^4.1.0",
"dom-accessibility-api": "^0.5.4",
"lz-string": "^1.4.4",
"lz-string": "https://github.com/pieroxy/lz-string.git",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we install from a tag/branch instead? We're basically giving up SemVer here which could lead to all sorts of different issues.

lz-string has been out for years now with 3 million downloads. Not saying that this means it's safe but it looks like it.

@kentcdodds Was license mismatch in dependencies ever a problem for you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can point to a specific commit rather than the main branch which is what I would recommend.

I've never had license issues before.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for your review, I just pushed a commit to add the lz-string latest commit hash on main branch

Copy link
Member

@MatanBobi MatanBobi Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's also not the best approach because now we're fixed to a specific release.
Maybe that's the best we can do though in regards to this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue was reported by my company's front-end npm's safe detection platform. Now, I use yarn resolutions to fixed @testing/library at 7.25.0 which not has lz-string as it's dependencies. Can you approve this mr and publish the new version then I can keep @testing/library to latest version, or there would have any other better solution for this issue? Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit hesitant to merge that since I don't know if every package manager can resolve that or creates other problems. Considering you already discovered yarn resolutions I would recommend using that for lz-string:

"resolutions": {
  "**/lz-string": "https://github.com/pieroxy/lz-string.git"
}

"pretty-format": "^26.6.2"
},
"devDependencies": {
Expand Down