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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Add type checking for cookies value #68

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
618eba4
Add type checking for cookies value.
greg2012201 Apr 6, 2024
6fe80c0
Merge branch 'andreizanik:master' into cookie-value-as-boolean
greg2012201 May 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const stringify = (value: string = '') => {
};

const decode = (str: string): string => {
if (!str) return str;
if (!str || typeof str !== 'string') return str;
Copy link
Owner

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

Copy link
Owner

@andreizanik andreizanik May 2, 2024 •
edited
Loading

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?

Copy link
Contributor Author

@greg2012201 greg2012201 May 4, 2024 •
edited
Loading

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.

Copy link
Owner

@andreizanik andreizanik May 10, 2024 •
edited
Loading

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.

Copy link
Contributor Author

@greg2012201 greg2012201 May 13, 2024 •
edited
Loading

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

Copy link
Owner

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


return str.replace(/(%[0-9A-Z]{2})+/g, decodeURIComponent);
};
Expand Down