repl: deprecate duplicated repl features #33294

BridgeAR · 2020-05-07T23:40:54Z

repl: deprecate repl.inputStream and repl.outputStream

The stream is exposed twice. As such it's best to rely upon the
.input and .output properties set by readline.

repl: deprecate repl._builtinLibs

This is a manually edited and outdated list of builtin modules.
Instead, it is better to rely upon the officially documented way
to get a list of builtin modules.

repl: remove obsolete completer variable

It is also assigned in readline. There is not need to assign the
variable twice.

repl: rewrite exports to module.exports

This makes sure all exports are in one place. Thus, it is easier to
see what parts are actually exported and which are not.

@nodejs/repl @nodejs/tsc PTAL

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2020-05-07T23:42:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31227/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2374/

addaleax

Our deprecation policy is – luckily – quite explicit now about the fact that aliases should be preferred to runtime deprecations.

As for _builtinLibs, the mismatches with builtinModules are intentional (on my end – can’t speak for you or others, obviously), partly in order to sort out the halfway public underscored modules like _http_outgoing, _tls_common, etc.

addaleax · 2020-05-07T23:48:35Z

Please have a look at the individual commit messages for details.

By the way:

$ git config --global alias.desc '!f() { git log --pretty="format:##### %s%n%n%b" upstream/master...HEAD --reverse; }; f'
$ git desc

Gives you a “nice” markdown printout of the commits on the current branch. I find it quite helpful when putting together PR descriptions for easier review.

doc/api/deprecations.md

BridgeAR · 2020-05-08T00:10:02Z

@addaleax thanks for the nice shortcut 👍

I just updated the deprecation to a doc-only deprecation which is allowed for aliases.

The builtinLibs are only exposed in the REPL and they are not up to date. They lack a few modules that should be listed. I think we should also update builtinModules to only list "actual" modules and not the underscored ones.

addaleax · 2020-05-08T00:12:19Z

@BridgeAR The two module lists have very different purposes:

repl._builtinLibs gives the list of modules that should be easily accessible in the REPL.
module.builtinModules gives the list of modules for which require(moduleName) will load a Node.js core module instead of one from the disk.

I think they’re fundamentally irreconceilable.

BridgeAR · 2020-05-08T00:17:30Z

If we keep _builtinLibs we should document it and name it builtinLibs. And builtinLibs should always rely upon builtinModules, otherwise we have an outdated list as we currently have (multiple modules are missing). It is possible to filter the underscores etc. that's actually what I do in #33282.

addaleax · 2020-05-08T00:19:20Z

@BridgeAR That sounds like a good approach to me too 👍

BridgeAR · 2020-05-08T00:23:48Z

@addaleax I would still deprecate _builtinLibs though. I am going to open a separate PR to add builtinLibs.

BridgeAR · 2020-05-08T00:44:30Z

@addaleax I just opened the other PR to document the property properly. Since I changed the deprecation to a doc-deprecation, is there still something that you disagree with?

BridgeAR · 2020-05-11T12:57:41Z

Ping @addaleax

addaleax · 2020-05-11T18:02:29Z

@BridgeAR I think this still runtime-deprecates _buildinLibs? Once we have #33295 – which I’m a fan of, obviously – it could just be an alias.

BridgeAR · 2020-05-11T18:24:31Z

@addaleax I changed it to a documentation-only deprecation as well. PTAL.

BridgeAR · 2020-05-11T18:31:31Z

I just updated the semverness to semver-minor, nice our deprecation documentation explicitly just mentions runtime and end-of-life deprecations being semver-major.

[Documentation-only] deprecations [...] are not breaking changes (semver-major)

doc/api/deprecations.md

The stream is exposed twice. As such it's best to rely upon the .input and .output properties set by readline. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

It is also assigned in readline. There is not need to assign the variable twice. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

targos · 2020-05-16T09:07:49Z

Landed in 2b50cd7...ab7d520

The stream is exposed twice. As such it's best to rely upon the .input and .output properties set by readline. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: #33294 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

It is also assigned in readline. There is not need to assign the variable twice. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: #33294 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This is a manually edited and outdated list of builtin modules. Instead, it is better to rely upon the officially documented way to get a list of builtin modules. As a side by fix this makes sure all exports are in one place. Thus, it is easier to see what parts are actually exported and which are not. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: #33294 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

aduh95 · 2020-05-16T09:38:04Z

@targos This has landed on master without changing the DEPXXXX to the actual ones (should be DEP0141 and DEP0142 I think):

node/doc/api/deprecations.md

Line 2671 in 48b1595

node/doc/api/deprecations.md

Line 2685 in 48b1595

Refs: nodejs#33294

The stream is exposed twice. As such it's best to rely upon the .input and .output properties set by readline. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: #33294 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

It is also assigned in readline. There is not need to assign the variable twice. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: #33294 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This is a manually edited and outdated list of builtin modules. Instead, it is better to rely upon the officially documented way to get a list of builtin modules. As a side by fix this makes sure all exports are in one place. Thus, it is easier to see what parts are actually exported and which are not. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: #33294 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs#33294 PR-URL: nodejs#33430 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Notable changes: async_hooks: * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891 cli: * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292 fs: * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134 http: * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119 repl: * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294 * (SEMVER-MINOR) remove obsolete completer variable (Ruben Bridgewater) #33294 * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294 * (SEMVER-MINOR) improve repl autocompletion for require calls (Ruben Bridgewater) #33282 * (SEMVER-MINOR) replace hard coded core module list with actual list (Ruben Bridgewater) #33282 * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282 * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282 src: * add support for TLA (Gus Caplan) #30370 test: * (SEMVER-MINOR) refactor test/parallel/test-bootstrap-modules.js (Ruben Bridgewater) #33282 PR-URL: TODO

Notable changes: async_hooks: * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891 cli: * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292 fs: * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134 http: * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119 repl: * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294 * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294 * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282 * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282 src: * add support for TLA (Gus Caplan) #30370 PR-URL: TODO

Notable changes: async_hooks: * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891 cli: * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292 fs: * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134 http: * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119 repl: * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294 * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294 * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282 * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282 src: * add support for TLA (Gus Caplan) #30370 PR-URL: #33452

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 7, 2020

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label May 7, 2020

addaleax requested changes May 7, 2020

View reviewed changes

jasnell reviewed May 7, 2020

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

jasnell reviewed May 7, 2020

View reviewed changes

doc/api/deprecations.md Show resolved Hide resolved

BridgeAR force-pushed the deprecate-repl-features branch 2 times, most recently from c58cd3a to f098ccd Compare May 8, 2020 00:07

addaleax approved these changes May 11, 2020

View reviewed changes

BridgeAR added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 11, 2020

BridgeAR added notable-change PRs with changes that should be highlighted in changelogs. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 11, 2020

This comment has been minimized.

Sign in to view

targos reviewed May 11, 2020

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

targos reviewed May 11, 2020

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

BridgeAR added 2 commits May 15, 2020 02:31

repl: deprecate repl.inputStream and repl.outputStream

b32f1ae

The stream is exposed twice. As such it's best to rely upon the .input and .output properties set by readline. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

repl: remove obsolete completer variable

bae3a62

It is also assigned in readline. There is not need to assign the variable twice. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

jasnell approved these changes May 15, 2020

View reviewed changes

targos closed this May 16, 2020

aduh95 added a commit to aduh95/node that referenced this pull request May 16, 2020

repl: update deprecation codes

ccce72f

Refs: nodejs#33294

aduh95 mentioned this pull request May 16, 2020

repl: update deprecation codes #33430

Closed

4 tasks

addaleax added dont-land-on-v10.x labels May 16, 2020

codebytere mentioned this pull request May 18, 2020

v14.3.0 proposal #33452

Merged

codebytere mentioned this pull request May 20, 2020

feat: handle unmarked DEP0XXX tags during landing and release nodejs/node-core-utils#420

Merged

aduh95 mentioned this pull request Sep 5, 2024

repl: runtime deprecate inputStream, outputStream and _builtinLibs #54750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repl: deprecate duplicated repl features #33294

repl: deprecate duplicated repl features #33294

BridgeAR commented May 7, 2020 •

edited

Loading

nodejs-github-bot commented May 7, 2020 •

edited by BridgeAR

Loading

addaleax left a comment

addaleax commented May 7, 2020

BridgeAR commented May 8, 2020

addaleax commented May 8, 2020

BridgeAR commented May 8, 2020

addaleax commented May 8, 2020

BridgeAR commented May 8, 2020

BridgeAR commented May 8, 2020

BridgeAR commented May 11, 2020

addaleax commented May 11, 2020

BridgeAR commented May 11, 2020

BridgeAR commented May 11, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

targos commented May 16, 2020

aduh95 commented May 16, 2020

repl: deprecate duplicated repl features #33294

repl: deprecate duplicated repl features #33294

Conversation

BridgeAR commented May 7, 2020 • edited Loading

repl: deprecate repl.inputStream and repl.outputStream

repl: deprecate repl._builtinLibs

repl: remove obsolete completer variable

repl: rewrite exports to module.exports

Checklist

nodejs-github-bot commented May 7, 2020 • edited by BridgeAR Loading

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented May 7, 2020

BridgeAR commented May 8, 2020

addaleax commented May 8, 2020

BridgeAR commented May 8, 2020

addaleax commented May 8, 2020

BridgeAR commented May 8, 2020

BridgeAR commented May 8, 2020

BridgeAR commented May 11, 2020

addaleax commented May 11, 2020

BridgeAR commented May 11, 2020

BridgeAR commented May 11, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

targos commented May 16, 2020

aduh95 commented May 16, 2020

BridgeAR commented May 7, 2020 •

edited

Loading

nodejs-github-bot commented May 7, 2020 •

edited by BridgeAR

Loading

BridgeAR commented May 11, 2020 •

edited

Loading