[KeyVault-Certificates] Rollup and test utils #4749
Conversation
sadasant
added
Client
sadasant
force-pushed
the
feature/rollupFor4623
branch
from
5254c4c to
fde5074
Compare
| dotenv.config({ path: "../.env" }); | ||
|
|
||
| export function isBrowser(): boolean { | ||
| return typeof window !== "undefined"; |
There was a problem hiding this comment.
Is it better to do this or just say !isNode()?
There was a problem hiding this comment.
Yes! I agree. However: This recorder will be removed soon. This feedback is probably relevant in the new core recorder too. Should I still do the change?
| export const isRecording = env.TEST_MODE === "record"; | ||
| export const isPlayingBack = env.TEST_MODE === "playback"; | ||
|
|
||
| // IMPORTANT: These are my attempts to make this more generic without changing it significantly |
There was a problem hiding this comment.
You probably want this comment not in the code but in the PR. Talk about what you changed and why you think the new version is better.
There was a problem hiding this comment.
This is a copy of the recorder.ts we have in keys and secrets. It will be replaced with a core recorder that Harsha is going to introduce soon. I can remove this comment if that's better!
| } | ||
|
|
||
| class NiseRecorder extends Recorder { | ||
| private readonly sasQueryParameters = ["se", "sig", "sp", "spr", "srt", "ss", "st", "sv"]; |
There was a problem hiding this comment.
This is taken from Harsha's code! I will ask him.
| for (const param in request.query) { | ||
| if ( | ||
| recording.query[param] === undefined && | ||
| !this.sasQueryParameters.includes(param) && |
There was a problem hiding this comment.
Are sasQueryParameters part of the KV work too?
There was a problem hiding this comment.
This is a copy of the recorder.ts we have in keys and secrets. It will be replaced with a core recorder that Harsha is going to introduce soon.
For this specific change, I would need a bit of time to understand what's happening here, but it will be wasted since the core recorder is coming soon.
Related to #4623 and #4624