Updates #17
Updates #17
Conversation
| "v8", | ||
| "vm", | ||
| "wasi", | ||
| "worker_threads", |
There was a problem hiding this comment.
I don't know why wasi was here, because even before my changes, running npm run make did not output it (tested in Node.js 6, 8, 10, 12, 14, 16, 17, 18). After some investigation I found that it is a module hidden under an experimental flag. If this flag is set, then this module will include it. However, I think it's better to omit it from the static list, since usually it will not be present.
| const ignoreList = [ | ||
| 'sys' | ||
| ]; | ||
|
|
||
| // eslint-disable-next-line node/no-deprecated-api | ||
| module.exports = (builtinModules || (process.binding ? Object.keys(process.binding('natives')) : []) || []) | ||
| .filter(x => !/^_|^(internal|v8|node-inspect)\/|\//.test(x) && !ignoreList.includes(x)) |
There was a problem hiding this comment.
Both using process.binding and accounting for ^internal/ are only necessary for Node.js 4 and below. Since they are already not supported by this package, so I just removed those, to simplify the logic.
| with: | ||
| node-version: ${{ matrix.node-version }} | ||
| - run: npm install | ||
| - run: npm test | ||
| test-static-list-is-up-to-date: |
There was a problem hiding this comment.
It would be better to do this as a cron job.
Example: sindresorhus/type-fest@ac845e9
There was a problem hiding this comment.
Do you recommend a schedule? Should I use the same? Also, I when this cron job fails, will you get a notification automatically?
There was a problem hiding this comment.
Yes, use the same. And yes, I will get a notification then.
| @@ -1,11 +1,6 @@ | |||
| 'use strict'; | |||
| const {builtinModules} = require('module'); | |||
|
|
|||
| const ignoreList = [ | |||
| 'sys' | |||
| ]; | |||
There was a problem hiding this comment.
I prefer to keep the ignore list as it's easier to add entries than a regex.
The existing regex could ideally be converted to a list like ignorePrefixList.
There was a problem hiding this comment.
Yeah I thought about ignorePrefixList, I was wondering which one you'd prefer, I guessed incorrectly, will update :)
| @@ -10,14 +10,31 @@ jobs: | |||
| fail-fast: false | |||
| matrix: | |||
| node-version: | |||
| - 18 | |||
|
Bump :) |
|
Bump |
I suspect you might have wanted to simply drop support for the old Node.js versions, but it was easy to not do it, so I left them all, I hope you don't mind. (If you want you can drop support for them on another release)
fs/promises#12I don't see anything here as breaking change.