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

Missing priority option. #274

Closed
2 tasks done
linkthai opened this issue Jan 16, 2024 · 7 comments · Fixed by #275
Closed
2 tasks done

Missing priority option. #274

linkthai opened this issue Jan 16, 2024 · 7 comments · Fixed by #275
Labels
bug Confirmed bug good first issue Good for newcomers

Comments

@linkthai
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.25.2

Plugin version

9.3.0

Node.js version

20.5.1

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

Sonoma

Description

The priority options was removed in 9.2.0, and the cookies are defaulted to priority Medium now.

9e4652f

Steps to Reproduce

reply.cookie(
      AUTH_COOKIE,
      JSON.stringify({
        accessToken,
        refreshToken,
        metadata,
        refreshTokenExpiresAt,
      }),
      {
        maxAge: new Date(refreshTokenExpiresAt).getTime() / 1000,
        httpOnly: true,
        sameSite: 'none',
        priority: 'high', <--- causing type error
      },
    );```

### Expected Behavior

The priority options have typings and it works.
@gurgunday
Copy link
Member

The type was there but I couldn’t find the real code, and assumed it didn’t exist, I’ll take another look

@gurgunday
Copy link
Member

Seems like that option didn’t really exist for a while now

I don’t see Priority defined in RFC 6265 either

Do you really need it?

@linkthai
Copy link
Author

Seems like that option didn’t really exist for a while now

Is it? In the 9.1.0, the cookie I created could use priority High and the cookie is actually on top of the cookie list. In the 9.3.0, the cookie is always Medium in the middle of the list.

I'm not sure if it 100% neccessary, but it's my authentication cookie. If I have an option to make its priority as high as I could I would do it.

I'm not acknowledged about RFC requirements, but Priority is an actual mechanism that is used by current browsers. And there is no mention or indication that they were deprecated, I can't find them anywhere (!). So I don't see why it was removed when it still being implemented by the browsers.

@climba03003
Copy link
Member

climba03003 commented Jan 17, 2024

The PR that removed the priority is #260 which we change from cookie to our own implementation.

We can change it back to using cookie as it support Partitioned now. or we should copy the same set of test to prevent regression on cookies handling.
@mcollina WDYT?

@gurgunday
Copy link
Member

gurgunday commented Jan 17, 2024

I vote on keeping the fork and adding it here

@mcollina
Copy link
Member

I think we should keep the fork at this point. We have been adding it back every now and then because of the slow responsiveness of cookie and unless we see some change there, I expect we would need to fork again in the future.

@climba03003 climba03003 added bug Confirmed bug good first issue Good for newcomers labels Jan 17, 2024
@Uzlopak
Copy link
Contributor

Uzlopak commented Jan 17, 2024

Created PR #275

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants