process: maintain constructor descriptor#9306
Conversation
Use the original property descriptor instead of just taking the value, which would, by default, be non-writable and non-configurable.
|
LGTM with green CI |
cjihrig
left a comment
There was a problem hiding this comment.
LGTM, but it's not entirely clear to me what you're hoping to accomplish with this change.
|
What's this aiming to solve? 🤔 |
|
@Fishrock123 an inconsistency in what's publicly provided in node. We're doing some automated iterating over node's public API objects, and inconsistencies can trip up our code. We can write in some exceptions, but it would be nice to not have to, especially in this case, as the non-configurability is not actually required here (it seems), and was just put there due to the way the property descriptor was originally used there. |
|
@Fishrock123 ... you good with this? |
|
ping @Fishrock123 |
|
Ping... status on this one? |
addaleax
left a comment
There was a problem hiding this comment.
LGTM, I see no problem with landing this.
|
Fresh CI: https://ci.nodejs.org/job/node-test-commit/8674/ Will land if it comes back green. |
|
Landed in e0bc5a7 |
Use the original property descriptor instead of just taking the value, which would, by default, be non-writable and non-configurable. PR-URL: #9306 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Use the original property descriptor instead of just taking the value, which would, by default, be non-writable and non-configurable. PR-URL: #9306 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
|
should this be backported? it lands cleanly. |
|
@MylesBorins I'd say yes. |
Use the original property descriptor instead of just taking the value, which would, by default, be non-writable and non-configurable. PR-URL: #9306 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Use the original property descriptor instead of just taking the value, which would, by default, be non-writable and non-configurable. PR-URL: nodejs/node#9306 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
process
Description of change
Use the original property descriptor instead of just taking the value,
which would previously, by default, be non-writable and non-configurable.
It didn't appear that it was intentional for
process.constructor's descriptorto be locked down, but it was, by using a simple
{value: ...}descriptor.