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

lance · 2016-03-14T20:52:17Z

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.

cjihrig · 2016-03-14T22:13:41Z

test/parallel/test-repl-use-global.js

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

jasnell · 2016-03-15T15:30:33Z

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

lance · 2016-03-15T16:09:47Z

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 · 2016-03-15T16:12:52Z

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 · 2016-03-28T15:28:23Z

@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 · 2016-04-25T19:36:52Z

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

addaleax · 2016-05-17T22:33:35Z

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

lance · 2016-05-18T18:02:09Z

@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 · 2016-05-18T18:20:15Z

@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 · 2016-05-18T18:52:46Z

@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 · 2016-05-18T18:53:49Z

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

addaleax · 2016-05-18T20:05:42Z

test/parallel/test-repl-use-global.js

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

addaleax · 2016-05-18T20:09:40Z

LGTM with nits addressed + pending CI

addaleax · 2016-05-18T21:48:45Z

@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 · 2016-05-19T13:43:18Z

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

lance · 2016-05-19T13:47:21Z

@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 · 2016-05-19T17:38:20Z

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 · 2016-06-30T14:20:20Z

LGTM if the CI is green.

lance · 2016-06-30T15:06:48Z

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

Yay! I will merge this today.

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 · 2016-06-30T17:08:42Z

Landed in: 15157c3

Fishrock123 · 2016-06-30T20:43:16Z

@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 · 2016-06-30T23:31:39Z

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

addaleax · 2016-07-01T08:37:38Z

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

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

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 · 2016-07-12T00:50:48Z

@addaleax @cjihrig LTS?

cjihrig · 2016-07-12T00:52:54Z

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

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 · 2016-07-20T22:06:17Z

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

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]>

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#5703Fixes: nodejs#7788 PR-URL: nodejs#7795 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

cjihrig · 2016-07-21T14:40:07Z

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

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: #5703Fixes: #7788 PR-URL: #7795 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

MylesBorins · 2016-08-30T14:12:56Z

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

cjihrig reviewed Mar 14, 2016
View reviewed changes

test/parallel/test-repl-use-global.js Outdated
Copy link
Contributor
cjihrigMar 14, 2016
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 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 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 15, 2016

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fbCompare April 26, 2016 05:23

addaleax mentioned this pull request May 17, 2016
REPL crashes after let process; #6802
Closed

addaleax reviewed May 18, 2016
View reviewed changes

addaleax mentioned this pull request May 28, 2016
doc: general improvements to repl.md copy #7002
Closed
2 tasks

repl: drop unnecessary assertion in useGlobal test
997eb87

lance closed this Jun 30, 2016

addaleax removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 1, 2016

Fishrock123 mentioned this pull request Jul 5, 2016
Propose v6.3.0 (v2) #7550
Merged

MylesBorins mentioned this pull request Jul 11, 2016
Audit commits not found on v4.x-staging #7661
Closed

MylesBorins added the lts-watch-v4.x label Jul 12, 2016

cjihrig mentioned this pull request Jul 19, 2016
Array and Object have different values in REPL and in files #7788
Closed

MylesBorins added the dont-land-on-v4.x label Jul 20, 2016

addaleax mentioned this pull request Aug 18, 2016
Cannot extend global prototypes in the REPL nodejs/node-v0.x-archive#771
Closed

MylesBorins removed the lts-watch-v4.x label Aug 30, 2016

Uh oh!

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

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

Uh oh!

Conversation

lance commented Mar 14, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

cjihrigMar 14, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

lance commented Mar 15, 2016

Uh oh!

cjihrig commented Mar 15, 2016

Uh oh!

Fishrock123 commented Mar 28, 2016

Uh oh!

lance commented Apr 25, 2016

Uh oh!

addaleax commented May 17, 2016

Uh oh!

lance commented May 18, 2016

Uh oh!

addaleax commented May 18, 2016

Uh oh!

lance commented May 18, 2016

Uh oh!

addaleax commented May 18, 2016

Uh oh!

addaleaxMay 18, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 18, 2016

Uh oh!

addaleax commented May 18, 2016

Uh oh!

Fishrock123 commented May 19, 2016

Uh oh!

lance commented May 19, 2016

Uh oh!

addaleax commented May 19, 2016

Uh oh!

cjihrig commented Jun 30, 2016

Uh oh!

lance commented Jun 30, 2016

Uh oh!

lance commented Jun 30, 2016

Uh oh!

Fishrock123 commented Jun 30, 2016

Uh oh!

cjihrig commented Jun 30, 2016

Uh oh!

addaleax commented Jul 1, 2016

Uh oh!

MylesBorins commented Jul 12, 2016

Uh oh!

cjihrig commented Jul 12, 2016

Uh oh!

MylesBorins commented Jul 20, 2016

Uh oh!

cjihrig commented Jul 21, 2016

Uh oh!

MylesBorins commented Aug 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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

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

MylesBorins commented Aug 30, 2016•
edited
Loading