qwik icon indicating copy to clipboard operation
qwik copied to clipboard

feat: First Class Cookie Support

Open nnelgxorz opened this issue 1 year ago • 5 comments

What is it?

  • [x] 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: https://github.com/withastro/astro/pull/4876

Fixes: #1851

Checklist:

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

nnelgxorz avatar Oct 24 '22 18:10 nnelgxorz

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

stackblitz[bot] avatar Oct 24 '22 18:10 stackblitz[bot]

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.

utherpally avatar Oct 24 '22 23:10 utherpally

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

nnelgxorz avatar Oct 25 '22 03:10 nnelgxorz

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. :+1:

nnelgxorz avatar Oct 28 '22 17:10 nnelgxorz

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. :+1:

nnelgxorz avatar Oct 28 '22 18:10 nnelgxorz

So awesome, great PR! Thanks @nnelgxorz!!

adamdbradley avatar Oct 31 '22 14:10 adamdbradley