-
Notifications
You must be signed in to change notification settings - Fork 10
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
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.
@@ -0,0 +1,432 @@ | |||
'use strict'; | |||
|
|||
/* eslint-disable node-core/prefer-primordials */ |
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.
This is identical to the primordials.js
in the Node.js codebase.
const { | ||
ArrayIsArray, | ||
ArrayPrototypeConcat, | ||
ArrayPrototypeIncludes, |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why *.js and not just a dot?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This address one of the linting issues, might as well make it so samples are copy/pastable.
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
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! Sometime this week, I'll make a PR removing primordials.js
and replacing the require from it with a package that does things properly for "not node core".
@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).