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.

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

The repl module's default setting for useGlobal invites hard-to-isolate bugs #13827

Closed
searls opened this issue Jun 20, 2017 · 3 comments
Closed
Labels
doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem.

Comments

@searls
Copy link

searls commented Jun 20, 2017

  • Version: v8.1.2
  • Platform: Darwin bayswater.local 16.6.0 Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64
  • Subsystem: repl

I think the built-in repl module's default option value of useGlobal: false is likely very surprising for many use cases, and should be reconsidered. For reference, the node CLI sets this value manually to true, so the REPL that Node users are accustomed to has their code run in a single shared context.

What I just discovered today, while working on testdouble.js with its little repl script is that the consequences of useGlobal defaulting to false are quite striking. We found a very insiduous bug that manifested itself as functions defined in the REPL not seeming to have Object or Function in their prototype chain (which, of course, every function does).

After lots of digging, we discovered it was due to the fact that useGlobal is false by default. For a minimal example of how absurd this seems, see the following output:

$ node -e 'require("repl").start()'
> setTimeout instanceof Function
false

Of course, it's ridiculous that setTimeout, or any built-in/host method would fail an instanceof check with Function. With the node CLI or the useGlobal option manually set to true, however, things behave much more akin to a real-world Node.js program.

$ node -e 'require("repl").start({useGlobal: true})'
> setTimeout instanceof Function
true
$ node
> setTimeout instanceof Function
true

As a result, it seems to me that useGlobal: true would have been a more sensible default for the built-in repl module, but since it's stable, maybe we can at least document the ramifications of this quirk in behavior. Thoughts?

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Jun 20, 2017
@bnoordhuis bnoordhuis added doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. labels Jun 21, 2017
@bnoordhuis
Copy link
Member

A documentation pull request would be welcome.

Of course, it's ridiculous that setTimeout, or any built-in/host method would fail an instanceof check with Function.

That's due to how stuff is copied over from the main context. Should probably be fixed but that's going to be a fair bit of work.

@benjamingr benjamingr added good first issue Issues that are suitable for first-time contributors. and removed good first issue Issues that are suitable for first-time contributors. labels Jun 21, 2017
searls added a commit to testdouble/testdouble.js that referenced this issue Jun 23, 2017
This was causing all our metaprogramming that checked types to break.
See: nodejs/node#13827
@vishwasrao
Copy link

Hi, I would like to work on this.
Eager to work on my first contribution.

@gibfahn
Copy link
Member

gibfahn commented Jul 13, 2017

@vishwasrao looks like @starkwang already raised a Pull Request for this (#13866).

I'll take off the good first contribution label

@gibfahn gibfahn removed the good first issue Issues that are suitable for first-time contributors. label Jul 13, 2017
addaleax pushed a commit that referenced this issue Jul 18, 2017
PR-URL: #13866
Fixes: #13827
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
PR-URL: #13866
Fixes: #13827
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants