Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Updates #17

Closed
wants to merge 5 commits into from
+63 −12
Closed

Updates #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
114310d
Simplify index.js
papb Oct 13, 2022
1aef3b5
Improve tests
papb Oct 13, 2022
62e5e9f
Add test-on-all-versions script using fnm
papb Oct 13, 2022
5027fce
Update builtin-modules.json (Node.js v18.10.0)
papb Oct 13, 2022
e65fa3e
Improve CI
papb Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 19 additions & 2 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,31 @@ jobs:
fail-fast: false
matrix:
node-version:
- 18
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node.js 19 introduced a new submodule.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodejs/node#44250

- 17
- 16
- 14
- 12
- 10
- 8
# Note: not easy to setup GitHub Actions correctly in this case for Node 6, but you can run the tests locally (test-on-all-versions.sh)
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- run: npm install
- run: npm test
test-static-list-is-up-to-date:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to do this as a cron job.

Example: sindresorhus/type-fest@ac845e9

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you recommend a schedule? Should I use the same? Also, I when this cron job fails, will you get a notification automatically?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, use the same. And yes, I will get a notification then.

name: Static list is up-to-date
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: latest
- run: npm install
- run: npm test
- run: mv builtin-modules.json builtin-modules-original.json
- run: npm run make
- run: diff builtin-modules.json builtin-modules-original.json
12 changes: 11 additions & 1 deletion builtin-modules.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
[
"assert",
"assert/strict",
"async_hooks",
"buffer",
"child_process",
Expand All @@ -10,9 +11,11 @@
"dgram",
"diagnostics_channel",
"dns",
"dns/promises",
"domain",
"events",
"fs",
"fs/promises",
"http",
"http2",
"https",
Expand All @@ -21,23 +24,30 @@
"net",
"os",
"path",
"path/posix",
"path/win32",
"perf_hooks",
"process",
"punycode",
"querystring",
"readline",
"readline/promises",
"repl",
"stream",
"stream/consumers",
"stream/promises",
"stream/web",
"string_decoder",
"timers",
"timers/promises",
"tls",
"trace_events",
"tty",
"url",
"util",
"util/types",
"v8",
"vm",
"wasi",
"worker_threads",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"zlib"
]
9 changes: 2 additions & 7 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
'use strict';
const {builtinModules} = require('module');

const ignoreList = [
'sys'
];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought about ignorePrefixList, I was wondering which one you'd prefer, I guessed incorrectly, will update :)


// 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))
Comment on lines -4 to -10
Copy link
Author

@papb papb Oct 13, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

module.exports = (builtinModules || [])
.filter(x => !/^_|^v8\/|^node-inspect\/|^sys$/.test(x))
.sort();
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
},
"scripts": {
"test": "xo && ava && tsd",
"test-on-all-versions": "./test-on-all-versions.sh",
"make": "node make.js"
},
"files": [
Expand Down
7 changes: 7 additions & 0 deletions test-on-all-versions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#!/usr/bin/env bash
set -Eeuo pipefail

for version in 6 8 10 12 14 16 17 18; do
fnm use "$version"
./node_modules/.bin/ava
done
25 changes: 23 additions & 2 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,13 @@ import test from 'ava';
import builtinModulesStatic from './static';
import builtinModules from '.';

test('main', t => {
console.log('Builtin modules:', builtinModules);
const nodejsMajorVersion = Number.parseInt(
process.version.split('.')[0].replace('v', ''),
10
);

test('builtinModules', t => {
console.log(`Builtin modules (Node.js ${nodejsMajorVersion}):`, builtinModules);

t.notThrows(() => {
for (const x of builtinModules) {
Expand All @@ -12,5 +17,21 @@ test('main', t => {
});

t.true(builtinModules.includes('fs'));

if (nodejsMajorVersion >= 14) {
t.true(builtinModules.includes('fs/promises'));
}

if (nodejsMajorVersion >= 16) {
t.true(builtinModules.includes('assert/strict'));
}

// Some Node.js versions (e.g v10) include some `v8/` and `node-inspect/` modules in `require('module').builtinModules`, but we want to ignore them
t.false(builtinModules.some(x => x.startsWith('v8/')));
t.false(builtinModules.some(x => x.startsWith('node-inspect/')));
});

test('builtinModulesStatic', t => {
t.true(Array.isArray(builtinModulesStatic));
t.true(builtinModulesStatic.every(x => typeof x === 'string'));
});