lib: add uncurried accessor properties to primordials#36329
lib: add uncurried accessor properties to primordials#36329ExE-Boss wants to merge 1 commit intonodejs:masterfrom
primordials#36329Conversation
|
I think that all property getters do not have a setter accessible to users. That would mean there is no "security" reason to add them to primordials, it's just more convenient to not duplicate calls to |
Modifying the code to count and log the number of encountered setters shows that there are 22 setters encountered by the |
Interesting, I stand corrected :) Can you also replace this instance in Lines 119 to 121 in 7d45dd9 |
9cc02c7 to
73e07da
Compare
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/738/console (queued, will 404 until it launches) |
|
It seems to introduce some performance regressions, but it seems to me it's unrelated code 🤔 Details |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@aduh95 There’s apparently a way to only restart the failed CI builds from the https://ci.nodejs.org page (e.g.: https://ci.nodejs.org/job/node-test-pull-request/34677/) without needing to rerun all CI builds: From #35811 (comment):
|
|
@ExE-Boss Can you rebase on top of c91e608 to fix the CI failure please? |
73e07da to
02dad91
Compare
|
@aduh95 Done. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
aduh95
left a comment
There was a problem hiding this comment.
IMHO this change makes the code harder to read. Would you agree to create a function to avoid repeating the code over and over:
function defineGetterAndSetter(desc, dest, prefix, key) {
const { enumerable, get, set } = desc;
Reflect.defineProperty(dest, `${prefix}Get${key}`, {
value: uncurryThis(get),
enumerable
});
if (set !== undefined) {
Reflect.defineProperty(dest, `${prefix}Set${key}`, {
value: uncurryThis(set),
enumerable
});
}
}fdcfeb0 to
4ec94c4
Compare
|
Landed in c83e599 |
Closes: nodejs#32127 PR-URL: nodejs#36329 Fixes: nodejs#32127 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Closes: #32127
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes