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: First Class Cookie Support #1864

Merged
merged 24 commits into from
Oct 31, 2022

Conversation

nnelgxorz
Copy link
Contributor

@nnelgxorz nnelgxorz commented Oct 24, 2022

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

The current headers implementation concatenates values with the same header using ", ". This works for all headers except Set-Cookie where commas are not allowed.

Other frameworks appear to rely on node-fetch having a raw() function that returns the non-concatenated header values. This seems like a poor assumption as we should not rely on a third party package being available inside various edge environments.

This PR creates a Cookie class that is accessible on the RequestContext. It allows a type-safe API for getting/setting cookies and allows us to dodge the header concatenation issue entirely.

Inspired by this Astro RFC: withastro/astro#4876

Fixes: #1851

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

@stackblitz
Copy link

stackblitz bot commented Oct 24, 2022

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

@utherpally
Copy link
Contributor

I think you can't use ', ' here

Set-Cookie: id=a3fWa; Expires=Wed, 21 Oct 2015 07:28:00 GMT

The <cookie-name> and <cookie-value> can't contains whitespace and comma but the Expires attribute can.

@nnelgxorz
Copy link
Contributor Author

Great catch, @langbamit! I think Svelte is using a special parser. I'll look into a better solution. Thanks!

@nnelgxorz nnelgxorz changed the title Allow appending multiple cookies feat: First Class Cookie Support Oct 28, 2022
Copy link

@ScottAgirs ScottAgirs left a comment

Choose a reason for hiding this comment

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

Nois, this is exciting!! ") Pls don't hate me for this question, but - any estimate if the could be a merge in near future? 😃

@nnelgxorz
Copy link
Contributor Author

Nois, this is exciting!! ") Pls don't hate me for this question, but - any estimate if the could be a merge in near future? smiley

Definitely the near future. 👍

@nnelgxorz
Copy link
Contributor Author

nnelgxorz commented Oct 28, 2022

Hey @manucorporat. I'm unable to run yarn api.update locally after rebasing because of this error:

packages/qwik/src/optimizer/src/plugins/vite.ts:252:11 - error TS2322: Type '{ outDir: string; rollupOptions: { input: string[]; preserveEntrySignatures: "exports-only"; output: OutputOptions; treeshake: { moduleSideEffects: false; }; onwarn: (warning: RollupWarning, warn: WarningHandler) => void; }; modulePreload: { ...; }; dynamicImportVarsOptions: { ...; }; }' is not assignable to type 'BuildOptions'.
  Object literal may only specify known properties, and 'modulePreload' does not exist in type 'BuildOptions'.

252           modulePreload: {
              ~~~~~~~~~~~~~~~~
253             polyfill: false,
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
254           },
    ~~~~~~~~~~~

  node_modules/vite/dist/node/index.d.ts:2448:5
    2448     build?: BuildOptions;
             ~~~~~
    The expected type comes from property 'build' which is declared here on type 'UserConfig'


Found 1 error in packages/qwik/src/optimizer/src/plugins/vite.ts:252

Any ideas? I could revert the change where I removed the cookies field from the Netlify Context and unblock this for merging.

Edit: Nevermind. Found a workaround. I'm leaving the error message so you're aware of the type error. 👍

@nnelgxorz nnelgxorz marked this pull request as ready for review October 28, 2022 18:07
@adamdbradley adamdbradley merged commit 94f7fed into QwikDev:main Oct 31, 2022
@adamdbradley
Copy link
Contributor

So awesome, great PR! Thanks @nnelgxorz!!

key: 'l',
value: '12',
options: { expires: new Date('Wed, 21 Oct 2015 07:28:00 GMT') },
expect: 'l=12; Expires=Wed, 21 Oct 2015 07:28:00 GMT',
Copy link

Choose a reason for hiding this comment

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

You should add a test for version 0 cookies.
i.e. if you have spaces it should be replaced by %20
e.g. 'l=foo%20bar; Expires=Wed, 21 Oct 2015 07:28:00 GMT'

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.

Allow appending multiple Set-Cookie Headers
5 participants