Skip to content

lib,repl: ignore canBeRequiredByUsers built-in#39942

Closed
XadillaX wants to merge 1 commit into
nodejs:masterfrom
XadillaX:add-builin-libs-to-object
Closed

lib,repl: ignore canBeRequiredByUsers built-in#39942
XadillaX wants to merge 1 commit into
nodejs:masterfrom
XadillaX:add-builin-libs-to-object

Conversation

@XadillaX
Copy link
Copy Markdown
Contributor

e.g. wasi under no --experimental-wasi-unstable-preview1 flag
shouldn't be pre-required.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Aug 30, 2021
@Mesteery
Copy link
Copy Markdown
Contributor

Fixes: #39911

@XadillaX XadillaX force-pushed the add-builin-libs-to-object branch from f61ea9e to 032c4f2 Compare August 30, 2021 10:20
Comment thread test/parallel/test-repl-built-in-modules.js Outdated
Comment thread test/parallel/test-repl-built-in-modules.js Outdated
@XadillaX XadillaX force-pushed the add-builin-libs-to-object branch from 032c4f2 to 5a4a555 Compare August 30, 2021 13:53
Comment thread test/parallel/test-repl-built-in-modules.js Outdated
Comment thread test/parallel/test-repl-built-in-modules.js Outdated
@XadillaX XadillaX force-pushed the add-builin-libs-to-object branch from 5a4a555 to 47bccb6 Compare August 30, 2021 16:46
Copy link
Copy Markdown
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

This might need to be semver-major.

});

return promise;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Most of this function seems to emulate what execFile does (promisified if a Promise is really necessary here).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

Comment thread test/parallel/test-repl-built-in-modules.js Outdated
Comment thread test/parallel/test-repl-built-in-modules.js Outdated
Comment thread test/parallel/test-repl-built-in-modules.js Outdated
@XadillaX XadillaX force-pushed the add-builin-libs-to-object branch 2 times, most recently from 98a64ec to 0d07117 Compare August 31, 2021 06:55
e.g. `wasi` under no `--experimental-wasi-unstable-preview1` flag
shouldn't be pre-required.
@XadillaX XadillaX force-pushed the add-builin-libs-to-object branch from 0d07117 to a9ee4eb Compare August 31, 2021 07:09
Copy link
Copy Markdown
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Copy Markdown
Member

@tniessen I also wonder about the semverness. So far most things in the REPL were not considered semver-major if it did not change the public API. Since it's mostly experimental libraries, I would be fine to call it a fix while I would go with semver-major if anyone would argue for it.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

XadillaX added a commit that referenced this pull request Sep 7, 2021
e.g. `wasi` under no `--experimental-wasi-unstable-preview1` flag
shouldn't be pre-required.

PR-URL: #39942
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
@XadillaX
Copy link
Copy Markdown
Contributor Author

XadillaX commented Sep 7, 2021

Landed in c7222b3

@XadillaX XadillaX closed this Sep 7, 2021
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
e.g. `wasi` under no `--experimental-wasi-unstable-preview1` flag
shouldn't be pre-required.

PR-URL: #39942
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem.

9 participants