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: Default useGlobal to false in CLI REPL. #5703

Closed
wants to merge 7 commits into from
Closed

repl: Default useGlobal to false in CLI REPL. #5703

wants to merge 7 commits into from

Conversation

lance
Copy link
Member

@lance lance commented Mar 14, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

repl

Description of change

This addresses #5659. The
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.


common.globalCheck = false;

test();
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't really needed. You can just inline test() :-)

@Fishrock123 Fishrock123 added repl Issues and PRs related to the REPL subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 15, 2016
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 15, 2016
@jasnell
Copy link
Member

jasnell commented Mar 15, 2016

Tentatively marking as semver-major due to the change in defaults.

@lance
Copy link
Member Author

lance commented Mar 15, 2016

Not sure if this is the place to ask this, but I couldn't find it addressed in CONTRIBUTING.md. Maybe someone on the thread can advise. What's the preferred method for pull request updates? Since I'm rebasing from upstream/master for each of these commits, I have to git push -f origin. I assume that makes incorporating the PR easier (vs. a merge which would end up inserting my changes back in the commit log instead of the tip. So that's what I've been doing.

But what about all of these incremental changes. Since I'm doing a git push -f origin anyway, all of the incremental commits could just be squashed into the original commit. Do you all prefer to see each commit as I respond to comments/line notes? Or would you rather have fewer commits, with all of the incrementals squashed into the original with each git push?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 15, 2016

It doesn't really matter while the PR is being reviewed. Once it is ready to be landed, it will need to be squashed into a single commit (or multiple commits, broken up logically, if it's a big PR).

@Fishrock123
Copy link
Contributor

@lance we all force-push to our branches at some point.

If you want to do incremental commits and squash later, that is fine. Sometimes I choose one or the other depending on the change.

@lance
Copy link
Member Author

lance commented Apr 25, 2016

Ping. Just checking to see if there is anything more that Committers would like to see addressed here - tidying up loose ends.

@addaleax
Copy link
Member

@lance Could you add something like a regression test for #6802 here?

@lance
Copy link
Member Author

lance commented May 18, 2016

@addaleax @cjihrig Note in the commit message, my comments. I am not sure how to test the failure case without causing the node test framework to fail.

@addaleax
Copy link
Member

@lance I don’t think you need to test the case where let process; is used with useGlobal = false anyway, because there’s no need to make sure that something is broken. If you are set on doing so, you can do that with a child process (see e.g. the tests from a7fd10e), and it would probably be best done in a new test file.

@lance
Copy link
Member Author

lance commented May 18, 2016

@addaleax just to be clear, the failure case is when useGlobal = true and code in the repl affects global. But I see what you mean. I will adjust the test.

@addaleax
Copy link
Member

@lance Eh, yeah, sorry :D I meant useGlobal = true, yep.

prompt: ''
};
repl.createInternalRepl(
process.env, opts, testFunc(useGlobal, cb, opts.output));
Copy link
Member

Choose a reason for hiding this comment

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

nit: double indent for statement continuations… or maybe just align the arguments vertically, I think I’d find that easier to read

@addaleax
Copy link
Member

LGTM with nits addressed + pending CI

@addaleax
Copy link
Member

@lance It might be a good idea to write a quick comment when you update PRs so that everyone can see that you did that. :)

CI: https://ci.nodejs.org/job/node-test-commit/3392/

and ping @nodejs/ctc because semver-major

@Fishrock123
Copy link
Contributor

Sorry, perhaps I have forgotten but I'm not particularly clear on what effect this will have.

@lance
Copy link
Member Author

lance commented May 19, 2016

@Fishrock123 currently, the CLI defaults useGlobal to true. This changes that default to false so that it is consistent with the documentation, and the defaults when a repl is created programatically. As a side effect, it seems to also fix #6802 in the default case.

@addaleax
Copy link
Member

The effect of this is going to be that the context in which Node’s standard REPL evaluates statements is different from the “default” one, in which virtually all other code, including the REPL’s code itself executes.

I think the biggest difference would be that changes occurring on the global object of the REPL don’t affect the rest of Node, which sounds desirable to me.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 30, 2016

LGTM if the CI is green.

@lance
Copy link
Member Author

lance commented Jun 30, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/3140/

Yay! I will merge this today.

lance added a commit that referenced this pull request Jun 30, 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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #5703
@lance
Copy link
Member Author

lance commented Jun 30, 2016

Landed in: 15157c3

@lance lance closed this Jun 30, 2016
@Fishrock123
Copy link
Contributor

@cjihrig Should we mark it as non-semver-major? we can always do a revert in v6.x if it doesn't work out

@cjihrig
Copy link
Contributor

cjihrig commented Jun 30, 2016

I would mark it as a bug fix for #6802 until proven otherwise. No tests were harmed while landing this PR.

@addaleax
Copy link
Member

addaleax commented Jul 1, 2016

Agreed, I’m taking the liberty to remove the semver-major label here.

@addaleax addaleax removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 1, 2016
Fishrock123 pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #5703
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
kunalspathak added a commit to kunalspathak/node-chakracore that referenced this pull request Jul 12, 2016
Earlier whenever repl was created (`node.exe` with zero parameters),
global context was reused. However nodejs/node#5703 fixed
it by creating new context for repl. This broke the chakrashim because the
context was getting collected immediately. The reason was we were adding
the reference of global context to the sandboxed object instead of newly
created context. Fixed it by ensuring we add reference to right context.
Also contextShim is always initialized when current context was pushed on
the scope. However for scenarios like this, we might just create the
context and access the objects like global, etc. of that context without
going to push scope path. In that case ensure that things are initialized
in the new context.
@MylesBorins
Copy link
Contributor

@addaleax @cjihrig LTS?

@cjihrig
Copy link
Contributor

cjihrig commented Jul 12, 2016

I would let it sit on v6 for a little while first. It almost seems too convenient that nothing broke :-)

cjihrig added a commit to cjihrig/node that referenced this pull request Jul 20, 2016
15157c3 changed the CLI REPL
to default to useGlobal: false by default. This caused the
regression seen in nodejs#7788.
This commit adds a known issue test while a proper resolution
is determined.

Refs: nodejs#5703
Refs: nodejs#7788
PR-URL: nodejs#7793
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins
Copy link
Contributor

due to #7788 I'm adding a dont-land label on this for now

evanlucas pushed a commit that referenced this pull request Jul 21, 2016
15157c3 changed the CLI REPL
to default to useGlobal: false by default. This caused the
regression seen in #7788.
This commit adds a known issue test while a proper resolution
is determined.

Refs: #5703
Refs: #7788
PR-URL: #7793
Reviewed-By: Rich Trott <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this pull request Jul 21, 2016
This is a partial revert of
15157c3. This change lead to a
regression that broke require() in the CLI REPL, as imported
files were evaluated in a different context.

Refs: nodejs#5703
Fixes: nodejs#7788
PR-URL: nodejs#7795
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Jul 21, 2016

Partially reverted in #7795. If you pull this and #7795 back to LTS, the net will only be two additional tests.

evanlucas pushed a commit that referenced this pull request Aug 2, 2016
This is a partial revert of
15157c3. This change lead to a
regression that broke require() in the CLI REPL, as imported
files were evaluated in a different context.

Refs: #5703
Fixes: #7788
PR-URL: #7795
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Aug 30, 2016

at this time I'm going to opt to not land these changes in LTS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants