[EngSys] upgrade dev dependency puppeteer to ^22.0.0
#28200
Conversation
by running `rush add -m --dev -p puppeteer@^21.0.0`
github-actions
bot
added
Azure.Core
Azure.Identity
EngSys
|
API change check API changes are not detected in this pull request. |
jeremymeng
force-pushed
the
engsys/puppeteer-v21
branch
from
db722d1 to
3ab2efe
Compare
jeremymeng
requested review from
ckairen,
deyaaeldeen,
minhanh-phan and
HarshaNalluru
as code owners
jeremymeng
requested review from
vicancy,
xirzec,
bterlson,
EmmaZhu,
XiaoningLiu,
vinjiang,
jiacfan,
ljian3377,
a team,
KarishmaGhiya,
maorleger,
witemple-msft and
schaabs
as code owners
| @@ -216,6 +217,12 @@ export class Recorder { | |||
| * - sanitizerOptions - Generated recordings are updated by the "proxy-tool" based on the sanitizer options provided, these santizers are applied only in "record" mode. | |||
| */ | |||
| async start(options: RecorderStartOptions): Promise<void> { | |||
| if (isBrowser && isPlaybackMode()) { | |||
There was a problem hiding this comment.
Were changes in the recorder related to the upgrade or something found in passing?
There was a problem hiding this comment.
Yes there's some behavior changes in headers being sent (Accept-Language). I updated sanitizers in recorder and keyvault. However some keyvault tests still failed.
There was a problem hiding this comment.
I need to re-record keyvault tests with the new service version some time in the next few days (it's not fully deployed yet). Hopefully just rerecording will fix the problem but if not I will investigate further at that time :)
There was a problem hiding this comment.
Also, I am curious what the change to Accept-Language was. I remember this being a bugbear for me when playing back tests in the past since the locale on my machine was en-CA instead of en-US which is what was present in the recordings.
There was a problem hiding this comment.
IIRC ideally the header should be ignored by test-proxy. Not sure what was the exact issue that prevented it from happening.
There was a problem hiding this comment.
We wanted to do this here Azure/azure-sdk-tools#6152 but didn't in the end, presumably due to engsys objection. @HarshaNalluru do you recall why?
There was a problem hiding this comment.
Their resistance was that it might have unintended consequences and is disruptive keeping other languages as well in mind.
From JS perspective, it should be safe, and we need to move forward with this change, so doing it in JS recorder makes sense.
jeremymeng
changed the title
puppeteer to ^21.0.0puppeteer to ^22.0.0
| @@ -216,6 +217,12 @@ export class Recorder { | |||
| * - sanitizerOptions - Generated recordings are updated by the "proxy-tool" based on the sanitizer options provided, these santizers are applied only in "record" mode. | |||
| */ | |||
| async start(options: RecorderStartOptions): Promise<void> { | |||
| if (isBrowser && isPlaybackMode()) { | |||
There was a problem hiding this comment.
Also, I am curious what the change to Accept-Language was. I remember this being a bugbear for me when playing back tests in the past since the locale on my machine was en-CA instead of en-US which is what was present in the recordings.
| @@ -216,6 +217,12 @@ export class Recorder { | |||
| * - sanitizerOptions - Generated recordings are updated by the "proxy-tool" based on the sanitizer options provided, these santizers are applied only in "record" mode. | |||
| */ | |||
| async start(options: RecorderStartOptions): Promise<void> { | |||
| if (isBrowser && isPlaybackMode()) { | |||
There was a problem hiding this comment.
Move this to a new method "preStart()" or similar, and call that here to keep the code clean?
| if (isPlaybackMode()) { | ||
| if (!this.httpClient) { | ||
| throw new RecorderError("httpClient should be defined in playback mode"); | ||
| } | ||
|
|
||
| await setMatcher(Recorder.url, this.httpClient, matcher, this.recordingId, options); | ||
| const excludedHeaders = isBrowser | ||
| ? (options.excludedHeaders ?? []).concat("Accept-Language") |
There was a problem hiding this comment.
Can you add a comment explaining why? for future reference
by running
rush add -m --dev -p puppeteer@^22.0.0Resolves #25790