search

bjoluc / next-redux-cookie-wrapper

Sync a subset of your Redux state with cookies in Next.js :cookie: :sparkles:
MIT License
114 stars 4 forks source link

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

Closed nielskrijger closed 2 years ago

nielskrijger commented 2 years ago

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):

https://github.com/pieroxy/lz-string/issues/125

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

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 commented 2 years ago

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 https://github.com/pieroxy/lz-string/issues/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 commented 2 years ago

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 commented 2 years ago

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!

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 2.1.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

nielskrijger commented 2 years ago

Thanks so much for this quick release! 🎉 👏

Can confirm it fixed the issue.