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

feat: Add support for custom serialization functions #41

Closed heinthanth closed 2 years ago

heinthanth commented 2 years ago

Description

This is an option to allow user provide custom encode / decode function instead of lzString(JSON.stringify(state)).

Reason

In NextJS, user might want to persist locale state using NEXT_LOCALE cookie. But as default, next-redux-cookie-wrapper decode the value en or some value as %22en%22. So, NextJS can't read the locale value and can't redirect as intended.

So, this feature allow user to pass custom encodeFunction: (state: any) => string and decodeFunction(state: string) => any options, so that they have control of how values are stored in cookie.

Documentation

Haven't written yet. Please give me suggestions about your opinion.

heinthanth commented 2 years ago

@bjoluc what do you think about this?

codecov[bot] commented 2 years ago

Codecov Report

Merging #41 (62c4479) into develop (170f07b) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           develop       #41   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          124       124           
  Branches        22        25    +3     
=========================================
  Hits           124       124           
Impacted Files Coverage Δ
main/src/index.ts 100.00% <ø> (ø)
main/src/config.ts 100.00% <100.00%> (ø)
main/src/cookies.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

bjoluc commented 2 years ago

Thanks for the PR @heinthanth! I've been expecting a request for such a feature and I'm more than happy to include it in the next release.

Since cookies are URI-encoded in many frameworks (including express) and the underlying cookie library defaults to URI-encoding cookies too, I'd suggest we leave the URI encoding in place. I think the problem here is not really the encoding but rather the serialization, so what about offering serializationFunction and deserializationFunction instead? This would decouple serialization and encoding and would leave the compression logic in place. For NEXT_LOCAL, the config would then be

{
  cookieName: "NEXT_LOCAL",
  serializationFunction: String, // defaults to `JSON.stringify`
  deserializationFunction: String, // defaults to `JSON.parse`
  compress: false,
}

WDYT?

I'll rebase your branch onto develop if you don't mind.

heinthanth commented 2 years ago

This is how NextJS respond to cookie.

image

Yes, You are right, the problem is due to serialization ( I didn't realized that before ).

my ( state ) => "my" ( JSON.stringify ) => %22my%22 ( encodeURIComponent )

When NextJS receive

%22my%22 ( cookie ) => "my" ( decodeURIComponent ) => So, NextJS locale detecting not working due to quotes.

Yes, u r right. We can leave URL encode as it's. So, I'll update my PR.

bjoluc commented 2 years ago

So, I'll update my PR.

Perfect! Would you mind rebasing it onto develop too then? I updated dependencies and a few code style things there already...

heinthanth commented 2 years ago

I don't mind. It's Ok.

bjoluc commented 2 years ago

I don't mind. It's Ok.

Means I should rebase it, or you rebase it? :stuck_out_tongue:

heinthanth commented 2 years ago

But there's some coverage issue. I'm bad at testing. I've no idea why that line is marked as uncovered ( I tested it tho ) Edit: that's right. I forgot to test with compress: false. Done 100% coverage.

@bjoluc I've update my PR. Please check and feel free to rebase.

For https://github.com/bjoluc/next-redux-cookie-wrapper/issues/17#issuecomment-1083942458, this PR still require lz-string if compress: true. I don't know which option is better. But user can use compress: false and use custom de/serialization functions.

bjoluc commented 2 years ago

@heinthanth What do you think about defaulting compress to false if serializationFunction or deserializationFunction are set? I guess that's what users need most commonly.

heinthanth commented 2 years ago

Yeah, it's OK too. Most users won't want to compress when they apply custom serializers.

bjoluc commented 2 years ago

Thanks @heinthanth!

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 2.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: