chore: install lz-string from git repo #932
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 651a12c:
|
Codecov Report
@@ Coverage Diff @@
## master #932 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 948 948
Branches 287 284 -3
=========================================
Hits 948 948
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
package.json
Outdated
| @@ -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", | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thx for your review, I just pushed a commit to add the lz-string latest commit hash on main branch
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"
}
|
Thanks for the work. I'm not comfortable with declaring git dependencies. Since there is a workaround (#932 (comment)) I'm closing. |
What:
I change lz-string install type from npm to git repo
Why:
lz-string@1.4.4 use WTFPL license, it could cause some law issue when use it.
How:
so I take the advice of it's author, install lz-stirng just from git repo to use MIT license. related issue
Checklist:
docs site