lib,doc: enable no-prototype-builtins #18750
Conversation
This enables the `no-prototype-builtins` eslint rule for /lib and for /doc.
nodejs-github-bot
added
assert
doc/api/util.md
Outdated
| @@ -65,7 +65,7 @@ is wrapped in an `Error` with the original value stored in a field named | |||
| callbackFunction((err, ret) => { | |||
| // When the Promise was rejected with `null` it is wrapped with an Error and | |||
| // the original value is stored in `reason`. | |||
| err && err.hasOwnProperty('reason') && err.reason === null; // true | |||
| err && Object.prototype.call(err, 'reason') && err.reason === null; // true | |||
There was a problem hiding this comment.
Object.prototype.hasOwnProperty.call?
|
CI https://ci.nodejs.org/job/node-test-pull-request/13140/ |
doc/api/util.md
Outdated
| @@ -65,7 +65,9 @@ is wrapped in an `Error` with the original value stored in a field named | |||
| callbackFunction((err, ret) => { | |||
| // When the Promise was rejected with `null` it is wrapped with an Error and | |||
| // the original value is stored in `reason`. | |||
| err && err.hasOwnProperty('reason') && err.reason === null; // true | |||
| console.log(err && Object.prototype.hasOwnProperty.call(err, 'reason') && | |||
There was a problem hiding this comment.
as long as this is being refactored can you pull hasOwnProperty off at the top like you did with the other files
There was a problem hiding this comment.
I went for removing as it is not necessary in the example. It is strictly testing for null and that has to be explicitly set.
BridgeAR
added
the
author ready
|
Worth noting this now relies on |
|
@apapirovski i'm making a pr based on discussions in #17434 which will (among other things) take care of that |
|
@devsnek Did we have a good conclusion out of that? I'm still not convinced I love any of the options... 😞 Having huge require blocks at the top for ObjectKeys, ReflectApply, ReflectKeys, etc. is not really where I want to be. But I mean, that's just all my opinion and it's fair enough if others feel differently. |
|
@apapirovski i'm opening the pr to hopefully heat things up a little bit and come to a final decision 😄 |
| @@ -400,7 +401,7 @@ function expectedException(actual, expected, msg) { | |||
| if (expected.prototype !== undefined && actual instanceof expected) { | |||
| return true; | |||
| } | |||
| if (Error.isPrototypeOf(expected)) { | |||
| if (isPrototypeOf.call(Error, expected)) { | |||
There was a problem hiding this comment.
Reflect.apply is preferred over Function.prototype.call/apply. Ditto everywhere else.
There was a problem hiding this comment.
I have one concern about using that in getOrSetAsyncId since it's somewhat hot path code... means creating an array for the arguments each time.
There was a problem hiding this comment.
in getOrSetAsyncId shouldn't we really be using an undefined check since it appears to be a lot faster
There was a problem hiding this comment.
@devsnek it still has the problem of potentially inheriting through prototype. I don't know how likely it is though... I'll ping @AndreasMadsen
There was a problem hiding this comment.
Reflect.has also seems significantly faster at this point, just as another option
|
I am feeling somewhat reluctant to merge this due to the discussion in #18773. What do others think? Shall I just close this? |
BridgeAR
removed
the
author ready
|
I am closing this for now as I feel the main discussion should be done first. |
This enables the
no-prototype-builtinseslint rule for /lib andfor /doc.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
lib,doc