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 true #7795

Merged
merged 1 commit into from
Jul 21, 2016
Merged

repl: default useGlobal to true #7795

merged 1 commit into from
Jul 21, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 19, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change

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.

A known issue test is available in #7793, which I'll port over here as a normal test.

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 19, 2016
@mscdex
Copy link
Contributor

mscdex commented Jul 19, 2016

s/globlal/global/ in the commit message

@silverwind silverwind changed the title repl: default useGloblal to true repl: default useGlobal to true Jul 19, 2016
@Fishrock123 Fishrock123 added the wip Issues and PRs that are still a work in progress. label Jul 19, 2016
@cjihrig cjihrig force-pushed the useglobal branch 2 times, most recently from 5859f88 to 4573a53 Compare July 20, 2016 18:13
@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 20, 2016

@mscdex typo fixed and added the regression test.

@addaleax
Copy link
Member

LGTM

1 similar comment
@JungMinu
Copy link
Member

LGTM

@addaleax
Copy link
Member

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 21, 2016

Sigh. Windows.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 21, 2016

Fixed the Windows slash issue. Ran the CI again: https://ci.nodejs.org/job/node-test-pull-request/3369/

Only one unrelated failure.

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 cjihrig merged commit 2cc01da into nodejs:master Jul 21, 2016
@cjihrig cjihrig deleted the useglobal branch July 21, 2016 14:38
@cjihrig cjihrig removed the wip Issues and PRs that are still a work in progress. label Jul 22, 2016
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]>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
@MylesBorins
Copy link
Contributor

I'm going to opt to not land this change on v4.x
@cjihrig feel free to change this if you think it should land

@lukechilds
Copy link
Contributor

lukechilds commented Oct 5, 2016

Sorry to bring up an old PR but I just came across this bug and it took me ages to get to the bottom of it. My project needs to work on 6.3 (deployment is beyond my control) so that's what I've been developing locally with. It's really confusing that my test suite runs ok but then if I manually run a test in the REPL it fails (due to requiring a module that modifies global).

Just wanted to check you were aware that this bug still occurs on 6.3.0 and 6.3.1. Would it make sense to release 6.3.2 with this fix so people using 6.3 don't get this issue or is there a reason this needs to be in 6.4?

@MylesBorins
Copy link
Contributor

@lukechilds unfortuantely the 6.3 line is no longer maintained. There have been a variety of security bugs that have been patched since. There will not be a new release on that line. While I can respect that the deployment is out of your control, do keep in mind that v6 will be going into LTS this month and any other releases in v6 will not be supported

@lukechilds
Copy link
Contributor

@thealphanerd I thought that might be the case, just checking. Thanks for the quick reply 👍

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