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

feat: decode callback parameter adds key #32

Closed
wants to merge 3 commits into from
Closed

Conversation

yxw007
Copy link
Contributor

@yxw007 yxw007 commented May 22, 2024

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Relate issuse:Decode function for useCookie called for every cookie present

Example:
image

Reason:
image
image

When useCookie('bar',..), document.cookie = "foo=FOO; bar=BAR", the parse function will traverse each cookie and then decode As a result, foo cookie will also call barCookie decode, which is unreasonable. So my idea is to return one more key field when parse, so that the upper layer can filter out the decode that does not belong to the current key.

@pi0
Copy link
Member

pi0 commented May 22, 2024

Thanks for this PR.

I think it is an interesting idea to pass the key object to the custom decode method but for the purpose of filtering the key to decode only, (#31) it might be overkill:

  • value normalization happens before we hit the custom decode function that can filter
  • Purpose of decode function should be decoding not handling filter logic. not only it makes usage more complicated and less readable but also might result in encouraging to diverge from cookie-es default decode behavior

Because of this i think we should think of a better way to explicitly support key filter

@pi0 pi0 marked this pull request as draft May 22, 2024 07:33
@yxw007
Copy link
Contributor Author

yxw007 commented May 22, 2024

Thanks for this PR.

I think it is an interesting idea to pass the key object to the custom decode method but for the purpose of filtering the key to decode only, (#31) it might be overkill:

  • value normalization happens before we hit the custom decode function that can filter
  • Purpose of decode function should be decoding not handling filter logic. not only it makes usage more complicated and less readable but also might result in encouraging to diverge from cookie-es default decode behavior

Because of this i think we should think of a better way to explicitly support key filter

You are right, it is indeed the case. I will think about it and see if there is a better way to solve it. Of course, if you have any good suggestions, please tell me.

@pi0
Copy link
Member

pi0 commented May 22, 2024

I think we can add new options.filter(key) option support and use it after L52

@yxw007
Copy link
Contributor Author

yxw007 commented May 22, 2024

I think we can add new options.filter(key) option support and use it after L52

image
image

May I ask if my understanding is correct ?

@pi0
Copy link
Member

pi0 commented May 22, 2024

Seems good only similar to js array filter() when a truehy value is returned i think we want to parse otherwise skip

@yxw007
Copy link
Contributor Author

yxw007 commented May 22, 2024

Seems good only similar to js array filter() when a truehy value is returned i think we want to parse otherwise skip

Okay, I'll adjust the judgment conditions

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

Successfully merging this pull request may close these issues.

2 participants