refactor: use primordials #37
Conversation
Modules in Node.js use primordial helpers, so that core APIs continue working if prototypes are mutated. By moving to using this same approach it will be easier to drop this module into Node.js once ready.
bcoe
requested review from
shadowspawn,
ljharb,
joesepi,
iansu and
darcyclarke
| @@ -0,0 +1,432 @@ | |||
| 'use strict'; | |||
|
|
|||
| /* eslint-disable node-core/prefer-primordials */ | |||
There was a problem hiding this comment.
This is identical to the primordials.js in the Node.js codebase.
| const { | ||
| ArrayIsArray, | ||
| ArrayPrototypeConcat, | ||
| ArrayPrototypeIncludes, |
There was a problem hiding this comment.
This prevents monkey patched prototypes on types like Array from breaking our core functionality, and is an approach used widely in the Node codebase.
There was a problem hiding this comment.
You may want to use https://npmjs.com/call-bind, and call-bind/callBound, for something published - node's own patterns only work because node can guarantee first execution for its primordials file, which this package can not.
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Ideally I would like to keep Could you submit a patch after this that modifies our Now that I think of it, having a separately published shim with the same exports as |
|
That’s a good idea. I’ll make such a package today or tomorrow, and then I’ll make a PR here to use it - that way index.js still can be drop-in (which was my thinking with the suggestion anyways; that call-bind would be used in primordials.js), but it’ll actually be robust when used outside of node core. |
package.json
Outdated
| @@ -6,8 +6,8 @@ | |||
| "scripts": { | |||
| "coverage": "c8 --check-coverage node test/index.js", | |||
| "test": "c8 node test/index.js", | |||
| "posttest": "eslint index.js", | |||
| "fix": "eslint index.js --fix" | |||
| "posttest": "eslint *.js", | |||
There was a problem hiding this comment.
Because it flags a bunch of issues with unused variables in README.md, seems like a nuisance to apply the same linting rules to js code blocks, since you know they'll be partial code.
There was a problem hiding this comment.
I’m confused; how could eslint possibly warn on md files if --ext=md isn’t passed?
Either way, we should disable vis eslintignore and/or overrides, not by limiting what gets linted (dot catches nested files, for example, *.js doesn’t)
There was a problem hiding this comment.
I’m confused; how could eslint possibly warn on md files if --ext=md isn’t passed?
Quick comment. Might be layered uses of eslint, like editor extension. I opened a PR prompted by md warnings in my environment! #35
There was a problem hiding this comment.
I just added README.md to ignores for now. I would have rather just added:
<!-- eslint-ignore no-unused-vars !-->
Tot he markdown file, which it seems should work based on markdown/markdown plugin information ... but none of the incantations I tried fixed it.
| @@ -119,6 +121,7 @@ const { flags, values, positionals } = parseArgs(argv, options); | |||
| ``` | |||
|
|
|||
| ```js | |||
| const { parseArgs } = require('@pkgjs/parseargs'); | |||
There was a problem hiding this comment.
This address one of the linting issues, might as well make it so samples are copy/pastable.
@ljharb sounds great, I think this repo will give us a good pattern we can copy for other work. |
Modules in Node.js use primordial helpers, so that core APIs continue working if prototypes are mutated. By moving to using this same approach it will be easier to drop this module into Node.js once ready.
I believe we will want to do some similar work as part of the resolution to #6, to make the errors we generate follow Node's conventions -- my hope is we can keep this module a direct mirror of the code in
utils/parseargs.js(in the Node codebase).