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

repl: useGlobal docs and code mismatch #5659

Closed
lance opened this issue Mar 11, 2016 · 22 comments
Closed

repl: useGlobal docs and code mismatch #5659

lance opened this issue Mar 11, 2016 · 22 comments
Labels
doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem.

Comments

@lance
Copy link
Member

lance commented Mar 11, 2016

The API documentation for REPL says,

"useGlobal - if set to true, then the repl will use the global object, instead of running scripts in a separate context. Defaults to false."

This is only true if a REPL is created programmatically. The REPL that is created by simply typing node on the command line defaults useGlobal to true.

This is because the internal createRepl function, used by node.js here, defaults this value to true. https://github.com/nodejs/node/blob/master/lib/internal/repl.js#L25

This can be verified by simply firing up a node REPL and typing .clear. You should see on the REPL command line, this text, "Clearing context....", but you don't.

  • Version: 4.2.4
  • Platform: Darwin Cerebus.local 15.3.0 Darwin Kernel Version 15.3.0: Thu Dec 10 18:40:58 PST 2015; root:xnu-3248.30.4~1/RELEASE_X86_64 x86_64
  • Subsystem: repl
@cjihrig
Copy link
Contributor

cjihrig commented Mar 11, 2016

I think I left a comment on your PR because of this. A documentation PR would be appreciated :-)

@Fishrock123 Fishrock123 added doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. good first issue Issues that are suitable for first-time contributors. labels Mar 11, 2016
@lance
Copy link
Member Author

lance commented Mar 11, 2016

@cjihrig I can submit a docs PR, but is the current behavior what's desired? Currently, the default is false if you create a REPL programmatically, but true if you start a REPL from the command line. It seems odd to have two different defaults.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 11, 2016

I don't really have a strong opinion, but if we're going to break something, my choice would be to have the command line REPL default to false too. I'm not sure off the top of my head what the implications would be outside of .clear.

@lance
Copy link
Member Author

lance commented Mar 11, 2016

OK, I will submit a separate PR for a change that defaults useGlobal to
false for the command line REPL

@Fishrock123
Copy link
Contributor

Doesn't this mean the CLI repl won't have access to any globals? i.e. require()? That is not an acceptable breaking change, and basically invalidates how the CLI repl is designed to work...

@Fishrock123
Copy link
Contributor

I don't think we should necessarily treat these things the same, although I think if there is no context provided to a programatic repl, it should probably use the global one.

Edit: actually, I don't think that is necessarily true... I don't think think the programatic and CLI repl should have to be the same, the purpose is not necessarily the same.

@lance
Copy link
Member Author

lance commented Mar 11, 2016

So, I've spent a while digging into how useGlobal works inside repl.js
and here's what I've found.

When a programmer writes code like this

const start = require('repl').start;
const repl = start({ prompt: 'REPL> ' });

the start function creates a new REPLServer object. The default
behavior in that constructor is to set the useGlobal value to
!!useGlobal, meaning that if it's not provided, then the default
value will be false. This would be the typical case, when using
the repl module programmatically, and any recommended change in
this issue should not change that behavior.

However, the REPL that is created by node.js when a REPL is started from the
command line uses the internal createInternalRepl
function exported from internal/repl.js. Here it sets useGlobal to true if an opts parameter is not provided, which is the case in the default usage. So, the command line REPL gets a useGlobal that is true.

So what does this mean? How does it affect the behavior in practice?
In either case, access to global variables such as require is available
because by the time this code is executing these have
already been set.
Since the useGlobal variable isn't exposed anywhere else, we can limit our search to repl.js.

In REPLServer.prototype.createContext, the flag is used to determine whether or not a new context should be created, or just use the current global. This function is used by the auto-completion bits, and by REPLServer.prototype.resetContext. The useGlobal boolean is also used to determine
whether or not to call resetContext when the .clear action is called in a REPL. And then finally, where it really matters, it's used by REPLServer's defaultEval function.

As an aside, it seems odd to me that these functions on REPLServer.prototype
are essentially public, but not documented. It would seem better to have them
either unavailable on the prototype, or documented here.
As a bonus, I think if createContext and resetContext were privately scoped,
most of this convoluted boolean checking would go away. Because ultimately
the value of useGlobal only determines one thing. It is used to determine
whether or not each statement in the REPL is evaluated
using script.runInThisContext (if useGlobal is true), or script.runInContext
(if useGlobal is false). I think its usage everywhere else is simply a byproduct
of these functions being exposed.

All of this is a long way of saying that I don't see this as a breaking
change, but I will agree that the complexity of this simple little boolean
value within repl.js is probably a lot greater than what it should be.

My personal opinion is that making the command line REPL match both the documented
behavior, and the default behavior when a REPLServer is created in code is
probably a good thing. It will not negatively impact users who are creating
REPLs in code because the default behavior there is not changing. It will
affect the REPL that is created by node on the command line. But it's not
entirely clear to me how other than the fact that a new context will be
created for statement evaluation instead of running in the initial context
created at node startup. But I'm a little hazy on that.

Edit: To be clear, I'm not suggesting that any response to this issue should involve changing the scope of functions on REPLServer.prototype. A change to the default CLI behavior seems pretty easy and simple, but changing those functions may be a can of worms.

@lance
Copy link
Member Author

lance commented Mar 14, 2016

The change for this is really simple - a one-liner. https://github.com/nodejs/node/blob/master/lib/internal/repl.js#L25 but I've been having a little trouble writing an automated test for it. If the real purpose of the boolean flag is to determine whether script.runInContext or script.runInThisContext is run, it's not clear how to best test that.

I know, first of all, that I'll need to have // Flags: --expose-internals in the test file in order to load internal/repl.js which contains the function needed for testing. But given that, is the code that is executing runInContext running in the same context as both the test and the code being executed by runInContext? And if so, by declaring a var foo; at the test level, should I expect this to be visible at the level of code being execute by runInContext?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 14, 2016

You should be able to differentiate between runInContext() and runInThisContext() by the presence of a global variable. According to the runInThisContext() documentation:

Running code does not have access to local scope, but does have access to the current global object.

@Fishrock123 Fishrock123 removed the good first issue Issues that are suitable for first-time contributors. label Mar 14, 2016
@lance
Copy link
Member Author

lance commented Mar 14, 2016

@cjihrig ok, got it. But let's say I have a test like this https://gist.github.com/lance/6a474ad892c34eed9026. I would expect this to pass. Maybe I misunderstand what is meant by useGlobal, because I am getting a failure here where global.lunch is still returning 'tacos'. Have I discovered another bug? Or am I just missing something obvious.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 14, 2016

When you set useGlobal to false, it still copies things from the global context into the new context in REPLServer.prototype.createContext(). In your example, try moving global.lunch = 'tacos'; into the createInternalRepl() callback. That should demonstrate the different behavior of useGlobal.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 14, 2016

Try this for your test:

'use strict';
const assert = require('assert');
const internalRepl = require('internal/repl');
const repl = require('repl');
const stream = require('stream');

function test(useGlobal, cb) {
  const inputStream = new stream.PassThrough();
  const outputStream = new stream.PassThrough();
  let output = '';

  outputStream.on('data', (data) => {
    output += data;
  });

  const opts = {
    input: inputStream,
    output: outputStream,
    useColors: false,
    terminal: false,
    useGlobal: useGlobal,
    prompt: ''
  };

  internalRepl.createInternalRepl(process.env, opts, function(err, repl) {
    if (err)
      return cb(err);

    global.lunch = 'tacos';
    repl.write('global.lunch;\n');
    repl.close();
    delete global.lunch;
    cb(null, output.trim());
  });
}

test(true, (err, output) => {
  assert.ifError(err);
  assert.strictEqual(output, '\'tacos\'');
  test(false, (err, output) => {
    assert.ifError(err);
    assert.strictEqual(output, 'undefined');
  });
});

@lance
Copy link
Member Author

lance commented Mar 14, 2016

@cjihrig thanks - I've got a working test now - realized my oversight. I will submit a PR this afternoon.

@lance lance closed this as completed in 15157c3 Jun 30, 2016
Fishrock123 pushed a commit that referenced this issue Jul 5, 2016
Documentation for REPL states that the default value of `useGlobal` is
`false`. It makes no distinction between a REPL that is created
programmatically, and the one a user is dropped into on the command line
by executing `node` with no arguments. This change ensures that the CLI
REPL uses a default value of `false`.

Fixes: #5659
Ref: #6802
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #5703
@cydhaselton
Copy link

I found this change after an alias I had set stopped working. Prior to the update, I could set:
alias='node -e repl.repl.ignoreUndefined=true -i'
in bashrc and run the default nodejs repl that ignored undefined. Post update I get a ReferenceError.

It's probably not a big enough issue to address, but it was the only way I could figure out how to modify the default repl. Running a script that starts a new repl instance doesn't help because the new repl doesn't have access to the default repl's history.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 21, 2016

@cydhaselton this was just reverted in #7795. A documentation PR might be appropriate just to note that the CLI REPL defaults to useGlobal: true.

@cydhaselton
Copy link

Thanks @cjihrig. Was there a release that incorporated the reversion?

@cjihrig
Copy link
Contributor

cjihrig commented Jul 21, 2016

Not yet. It was just reverted an hour ago :-)

@cydhaselton
Copy link

Apologies…reviewing from tablet. Missed the timestamps :-/

@cydhaselton
Copy link

Related question: is there a quick way to tell if a release contains a reversion?

@cjihrig
Copy link
Contributor

cjihrig commented Jul 22, 2016

The quickest way I can think of would be checking the changelog or release blog post.

@cydhaselton
Copy link

I did…the latest only has changes and commits. Do they usually show reversions/fixes, if any?

@cjihrig
Copy link
Contributor

cjihrig commented Jul 22, 2016

Yes. They should show all commits, including reverts. It looks like 2cc01da didn't make it into the v6.3.1 release, which was already in progress. It should be in the next release though.

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

4 participants