Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
[v18.x backport] url: ensure getter access do not mutate observable symbols#48891
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
aduh95 commented Jul 23, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Jul 23, 2023
Review requested:
|
aduh95 commented Jul 23, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1354/ Results |
The test's assumptions about RSS are no longer valid, at least with Fedora 38. Closes: nodejs#48490 PR-URL: nodejs#48811Fixes: nodejs#48490 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>
aduh95 commented Jul 23, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Benchmark results are mixed, but I think it makes sense to move forward with this change. |
PR-URL: #48897 Refs: #48891 Refs: #48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: nodejs#48897 Refs: nodejs#48891 Refs: nodejs#48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
c73411e to eb29d9aCompare
LiviaMedeiros left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/ or w/o nits
| functionisURLSearchParams(self){ | ||
| returnself&&self[searchParams]&&!self[searchParams][searchParams]; | ||
| returnself?.[searchParams]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| returnself?.[searchParams]; | |
| returnBoolean(self?.[searchParams]); |
| returnthis[searchParams]; | ||
| constcachedValue=internalSearchParams.get(this); | ||
| if(cachedValue!=null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if(cachedValue!=null) | |
| if(cachedValue!==undefined) |
PR-URL: nodejs#48897 Refs: nodejs#48891 Refs: nodejs#48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: nodejs#48897 Refs: nodejs#48891 Refs: nodejs#48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: nodejs#48897 Refs: nodejs#48891 Refs: nodejs#48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: nodejs#48897 Refs: nodejs#48891 Refs: nodejs#48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: nodejs#48897 Refs: nodejs#48891 Refs: nodejs#48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: nodejs#48897 Refs: nodejs#48891 Refs: nodejs#48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #48897 Refs: #48891 Refs: #48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
nodejs-github-bot commented Aug 16, 2023
ruyadorno commented Aug 16, 2023
@aduh95 Is this ready to land on v18.x? |
anonrig commented Aug 16, 2023
Yes it is ready. |
PR-URL: #48897 Backport-PR-URL: #48891 Refs: #48891 Refs: #48886 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
ruyadorno commented Aug 16, 2023
Landed in 0beb5ab |
Opening as draft so we can run benchmarks, if the perf looks OK, I'll open another PR to land the test on
mainfirst.Refs: #48886