-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add type checking for cookies value #68
Add type checking for cookies value #68
Conversation
@@ -53,7 +53,7 @@ const stringify = (value: string = '') => { | |||
}; | |||
|
|||
const decode = (str: string): string => { | |||
if (!str) return str; | |||
if (!str || typeof str !== 'string') return str; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for your PR but I think we have a problem with setCookie
function and only on backend side.
For example, if we run this code on frontend side everything will be ok
setCookie('MY_TEST_COOKIE', true);
console.log('MY TEST COOKIE HERE', getCookie('MY_TEST_COOKIE'));
and
console.log(typeof getCookie('MY_TEST_COOKIE')) === string
I think we need to figure out why a boolean value is set and not a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured it out a little and I think the problem is in line 109
It seems we need to use stringify
function
const payload = { name: key, value: stringify(data), ...restOptions };
like in 132 line
const cookieStr = serialize(key, stringify(data), { path: '/', ..._cookieOptions });
What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review!🙌 I understand your perspective. In this PR (after my changes), setting cookies as a boolean or even another type like a number works correctly: the value is being set (server-side), and you can easily get it (server-side), but only with the same type as it was set with. Let's examine these examples:
// 1
setCookie("MY_TEST_COOKIE", true, { req });
console.log("MY TEST COOKIE HERE", typeof getCookie("MY_TEST_COOKIE", { req })); // boolean
// 2
setCookie("MY_TEST_COOKIE", 10, { req });
console.log("MY TEST COOKIE HERE", typeof getCookie("MY_TEST_COOKIE", { req })); // number
// 3
setCookie("MY_TEST_COOKIE", 'test string', { req });
console.log("MY TEST COOKIE HERE", typeof getCookie("MY_TEST_COOKIE", { req })); // string
Setters and getters use cookies
from next/headers
and the req
object from Next.js
. They also behave accordingly.
Therefore, my proposal is to merge this PR as a fix for setting values server-side and either modify the rest of the codebase to adhere to the native Next.js behavior or parse every type of value to string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delay.
Cookies value is always string.
Your example works as you say because you try to set and to get cookies during one request
For example. Let's imagine two API endpoints
// api/set-cookie.ts
export const GET = async (req: NextRequest) => {
const res = new NextResponse();
setCookie('test', true, { res, req });
console.log(typeof getCookie('test', { res, req })); // ---> boolean
return res;
};
// api/get-cookie.ts
export const GET = async (req: NextRequest) => {
const res = new NextResponse();
console.log(typeof getCookie('test', { res, req })); // ---> string
return res;
};
I think this behavior can lead unexpected results so better way is to always set string and always get string
Also, without "app router" this library transform value to string before setting. It will be better to keep the same behavior for different cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I got It! 🙌 I have submitted another PR where all values are set as strings. Please check it out and test it -> #69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think this PR should be closed
Hello!
I have fixed a bug regarding this issue #66. After this fix, the bug described in the mentioned issue shouldn't appear anymore.