fix: cookies correlation #83
Conversation
This reverts commit 44628e9affdc90d55a113c629e528ac1df7c5530.
src/codegen/codegen.ts
Outdated
| @@ -152,10 +152,10 @@ function generateRequestParams(request: ProxyData['request']): string { | |||
| return ` | |||
| { | |||
| headers: { | |||
| ${request.headers.map(([name, value]) => `'${name}': '${value}'`).join(',\n')} | |||
| ${request.headers.map(([name, value]) => (name !== 'Cookie' ? `'${name}': \`${value}\`` : '')).join(',\n')} | |||
There was a problem hiding this comment.
Looks like this can generate invalid code, because a Cookie header will produce an extra comma:
headers {
headerA: '...',
,
headerB: '...',
}Here's what you can do:
request.headers
.filter(([name]) => name !== 'Cookie')
.map(([name, value]) => `'${name}': \`${value}\``)
.join(',')
| } | ||
| } | ||
| ` | ||
| } | ||
|
|
||
| // @ts-expect-error we have commonjs set as module option |
There was a problem hiding this comment.
Out of scope for this PR, but according to the typescript docs,
You very likely want "nodenext" for modern Node.js projects and preserve or esnext for code that will be bundled.
Should we try that out?
There was a problem hiding this comment.
indeed! I wanted to discuss the commonjs setting and what would be the appropriate one to use as I feel you both know more than me on the topic.
I've marked in this PR and others the error with the commonjs keyword so that when we make a decision it should be easy to remove these extra lines
| } | ||
| } | ||
| ` | ||
| } | ||
|
|
||
| // @ts-expect-error we have commonjs set as module option | ||
| if (import.meta.vitest) { |
There was a problem hiding this comment.
I don't mind in source testing, but we already have tests for codegen.ts in codegen.test.ts - isn't it a better place for these tests as well? At least for now.
There was a problem hiding this comment.
Unless I missunderstood something I would say nope.
codegen.test.ts makes sense to test the exported functionality or have bigger components tests.
In source makes sense in here because of the locality of the function and it being private (limited to testing the unit).
I guess we can export it and move the test there, but I don't see in-source & filename.test.ts replacing one another any time soon. They are complementary.
With that said, no strong opinions, do you think it's better here to export the function and test it in codegen.test.ts anyways ? 🤔
There was a problem hiding this comment.
I don't have a strong opinion either. Your explanation sounds good 👍
This fixes the issue where
cookiesandheaderswere not correctly correlated as the variable was not in a backticks string.Other major changes are:
headersare now excluding theCookiefield in favor of the k6cookiesparametercookiesare being defined explicitlyRegarding the
cookiespoint, it means that now the script is really verbose due to cookies needing an object specifyingreplacefor making it work withk6.This is because k6 has a
cookiejarthat automatically saves cookies and reuse them in further request, this was interfering with our own cookies making them duplicated, hence thereplace: true.I think this is good enough for now since it makes the application work but I would like to have a better solutions for cookies when we have time, for example we could rely always on the
cookiejarand only when a specific cookie is specified/modified we selectively target that one explicitly. It will need more brainstorming and thoughts.