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

LZ string: network request to pieroxy/lz-string/tar.gz/b2e0b270a9f3cf330b778b777385fcba384a1a02 failed #20

Closed
nielskrijger opened this issue Nov 22, 2021 · 5 comments
Labels

Comments

@nielskrijger
Copy link

nielskrijger commented Nov 22, 2021

Thank you for this library!

I plugged it in today and got it running by simply copying the README example. Compared to redux-persist it's very easy to setup.

Sadly I ran into an issue with our CI/CD which restricts NPM packages to be published on a registry. Meaning; external paths are blocked.

The lz-string dependency from this lib gets pulled-in through using:

"lz-string": "github:pieroxy/lz-string#b2e0b27",

I don't know why this library needed this specific version, I checked the git history but couldn't find a cause. One reason might be that the lz-string lib hasn't been published for a couple of years (this is just a guess):

pieroxy/lz-string#125

Having the github in package.json instead of normal semver has two consequences for my project:

  • lz-string was already a dependency for several other packages all using the latest published v1.4.4. Now it's pulled-in a second time I think.
  • our CI/CD won't download it

I have found a workaround by using force-resolution; https://www.npmjs.com/package/npm-force-resolutions

That workaround is a bit of a last-resort tbh. I've turned compression off which effectively makes lz-string an optional dependency I think. It's unlikely I'll ever hit the cookie limit for our use-case.

I'm not sure how this can be resolved now that dependents of this library use this unreleased version of lz-string. The only idea I had was keeping the lz-string dep as-is but change it to an optional dependency so people who don't need it can opt to exclude it (https://docs.npmjs.com/cli/v8/configuring-npm/package-json#optionaldependencies).

I figured I'd open this issue if others run into the same issue.

@bjoluc
Copy link
Owner

bjoluc commented Nov 22, 2021

Hi Niels,

thanks for the kind words, and I'm glad to hear the lib helps you!

Your guess is right about lz-string – I added the dependency according to pieroxy/lz-string#147, hoping that a new version would be released soon. Since I was wrong about that, and since it causes trouble for you, I think it's best to just use v1.4.4 from NPM.

I'm not sure how this can be resolved now that dependents of this library use this unreleased version of lz-string. The only idea I had was keeping the lz-string dep as-is but change it to an optional dependency so people who don't need it can opt to exclude it (https://docs.npmjs.com/cli/v8/configuring-npm/package-json#optionaldependencies).

I'm not sure if I get your point with this – I would simply do a patch release with the dependency definition changed to "lz-string": "^1.4.4". Users who update next-redux-cookie-wrapper will automatically be provided with lz-string v1.4.4 from the NPM registry then. I can't think of a scenario where the dependency change would break anything, unless someone is concerned about licenses, but then again, the WTFPL license in the NPM package is just false information. Am I missing an edge case?

I've turned compression off which effectively makes lz-string an optional dependency I think.

You may be right about not needing lz-string, though I don't know if next-redux-cookie-wrapper would currently work without it. The way to go about optional dependencies would most likely be using peerDependencies. Anyway, since compression is the default behavior, I'd like to keep lz-string a non-optional dependency for simplicity, given that it is quite slim.

Let me know what you think about a patch release :)
Cheers!

@nielskrijger
Copy link
Author

Hi @bjoluc

I'd like to keep lz-string a non-optional dependency for simplicity, given that it is quite slim.

Fair enough, it would indeed add complexity.

I'm not sure how this can be resolved now that dependents of this library use this unreleased version of lz-string.

I'm not sure if I get your point with this

Sorry, the only reason I said that was I hadn't looked at all commits that got merged in lz-string since the release of 1.4.4 and commit b2e0b27 used in this lib.

My concern was that the compression format might have changed in some subtle way that would break if you'd downgraded the lib. I.e. upgrade v1.4.4 -> #b2e0b27 is fine, but the reverse #b2e0b27 -> v1.4.4 wouldn't. For example if #b2e0b27 would introduce something new to the compression format that v1.4.4 doesn't understand yet. It's a bit far-fetched but that's the only reason I said that.

I walked through the commits in lz-string, most seem to be about documentation and superficial changes. Found two commits that had some code in them:

I think these are non-consequential as long as this lib can work with it.

After scanning those commits I'd think it's fine to patch-release to 1.4.4. It would definitely solve my issue 👏

@bjoluc
Copy link
Owner

bjoluc commented Nov 22, 2021

My concern was that the compression format might have changed in some subtle way that would break if you'd downgraded the lib.

Oh, right, makes sense. I assumed that there were no breaking changes on main, which (luckily) seems to be right, according to your research – thanks!

bjoluc added a commit that referenced this issue Nov 22, 2021
… the GitHub repo

Please take a look at #20 for the corresponding conversation

Closes #20
bjoluc added a commit that referenced this issue Nov 22, 2021
… the GitHub repo

Please take a look at #20 for the corresponding conversation

Closes #20
bjoluc added a commit that referenced this issue Nov 22, 2021
… the GitHub repo

Please take a look at #20 for the corresponding conversation

Closes #20
@bjoluc bjoluc closed this as completed in f6c8d09 Nov 23, 2021
github-actions bot pushed a commit that referenced this issue Nov 23, 2021
### [2.1.1](v2.1.0...v2.1.1) (2021-11-23)

### Bug Fixes

* **Dependencies:** Install lz-string from the NPM registry instead of the GitHub repo ([f6c8d09](f6c8d09)), closes [#20](#20) [#20](#20)
@github-actions
Copy link

🎉 This issue has been resolved in version 2.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nielskrijger
Copy link
Author

Thanks so much for this quick release! 🎉 👏

Can confirm it fixed the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants