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

fix(nuxt): use max length + iterations for useCookie timeout #24253

Merged
merged 5 commits into from Nov 20, 2023

Conversation

ChrisGV04
Copy link
Contributor

πŸ”— Linked issue

Resolves #24227

❓ 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

This PR fixes an issue introduced when implementing the functionality to make expired cookies undefined on the client-side. Since it uses setTimeout to apply the delay to expire the cookie, when you pass a long value to maxAge or expires, it deletes the cookie immediately instead of waiting that time.

This happens because of the maximum delay value allowed by browsers on setTimeout as stated in MDN and that @positiveprogrammer brought to my attention. The issue started happening on v3.8.1 with PR #23549.

In my case, I was setting a cookie that stores if the user consents to using cookies. That cookie choice should last 30 days, which is above the 24.8 day limit of setTimeout so if I updated the value client-side, the cookie instantly became undefined.

My proposed solution to work around this issue (since it's a browser limitation unrelated from Nuxt) is to implement extra logic to divide the task into smaller intervals that add up to the total expected delay. That way we never exceed the timeout limit and still wait the desired amount of time before expiring the cookie.

For example: If the maximum delay value was 5 seconds and you want a cookie with maxAge of 12 seconds, we are going to need 2 intervals of 5 seconds (the max value) and a timeout for the remaining 2 seconds. That way we add up to 12 seconds as expected (5 + 5 + 2 = 12) without exceeding the browser limit of 5 seconds of this example.

I'm not sure if this is the best possible solution to the problem, but it seems to work.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

stackblitz bot commented Nov 11, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@ChrisGV04 ChrisGV04 changed the title fix(nuxt): Prevent useCooke deleting cookies with long expiration client-side fix(nuxt): Prevent useCookie deleting cookies with long expiration client-side Nov 11, 2023
@ChrisGV04 ChrisGV04 changed the title fix(nuxt): Prevent useCookie deleting cookies with long expiration client-side fix(nuxt): prevent useCookie deleting cookies with long expiration client-side Nov 11, 2023
Copy link
Member

@huang-julien huang-julien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could probably wrap this with import.meta.client.

@ChrisGV04
Copy link
Contributor Author

ChrisGV04 commented Nov 12, 2023

Hi @huang-julien! How do you mean? Wrap the the whole if (intervalsNeeded)?

I enabled code contributions, so feel free to add it wherever you consider it appropriate.

@danielroe
Copy link
Member

(I think we probably don't need to wrap it again as we should only use the cookieRef on the client-side anyway.)

I think we can simplify this slightly. Maybe just keep track of a single timeElapsed number, and use a constant for setting a timeout?

@ChrisGV04
Copy link
Contributor Author

ChrisGV04 commented Nov 13, 2023

Hi @danielroe! Sounds good.

However, I took a closer look and I think there's a fundamental flaw in this functionality to set expired cookies to undefined on the client overall, since we always use the original maxAge and expires to set the timeout, not the actual time that the cookie expires at.

For example, if I set the cookie to expire 5 days from now, when I use the app on day 5 and read the cookie client-side, the timeout is being set to another 5 days, but the cookie only has a few hours left to expire.

If I understand correctly, this is also a browser limitation since there is no easy way to get the actual expires date of the cookie on the browser. I don't know if there's a smarter solution to all of this

@ChrisGV04
Copy link
Contributor Author

Hi @danielroe @huang-julien! I know you must be very busy.

What do you think about my last comment about the flaw with expiring the cookie client-side? Maybe you understand the subject better than I do.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests βœ…

❗ No coverage uploaded for pull request base (main@5d6cfd5). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24253   +/-   ##
=======================================
  Coverage        ?   58.76%           
=======================================
  Files           ?        5           
  Lines           ?      861           
  Branches        ?       46           
=======================================
  Hits            ?      506           
  Misses          ?      355           
  Partials        ?        0           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@danielroe
Copy link
Member

Pushed a refactor. Let me know what you think.

You are right about that potential issue; the behaviour is safe in that it shouldn't expire a cookie too early, but the cookie can remain accessible via a useCookie called before the expiration after it actually expires.

Ideas welcome to resolve, but we can also defer that to a subsequent PR.

@ChrisGV04
Copy link
Contributor Author

ChrisGV04 commented Nov 20, 2023

Hi @danielroe!

I believe this solves the current issue at hand, so it's good for now. Thank you very much!

You are right about the other issue, at least it won't expire early. I haven't been able to find a simple solution that doesn't involve storing the expiration date for each cookie.

Since it seems to be impossible to know that real expiration value, we either have to create an additional cookie or local storage that stores the expiration for every cookie created with useCookie or store the value and the expiration date as a JSON object inside the same cookie, but that would break using the cookie with other external services.

@danielroe danielroe changed the title fix(nuxt): prevent useCookie deleting cookies with long expiration client-side fix(nuxt): use max length + iterations for useCookie timeout Nov 20, 2023
@danielroe danielroe merged commit a10e33c into nuxt:main Nov 20, 2023
33 checks passed
@github-actions github-actions bot mentioned this pull request Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useCookie with maxAge or expires gets removed when updated
5 participants