GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — repl: don't override all internal repl defaults by cjihrig · Pull Request #7826 · nodejs/node

Conversation

@cjihrig

@cjihrig cjihrig commented Jul 21, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change

The createInternalRepl() method accepts an options object as an argument. However, if one is provided, it overrides all of the default options. This PR applies the options object to the defaults, only changing the values that are explicitly set.

The approach in this PR also adds a method to test/common.js, allowing specific global variables to be whitelisted. This is because the CLI REPL sets useGlobal to true, and then defines module and require() as globals. Alternative approaches could be to set useGlobal to false (which was what was silently happening before) or set common.globalCheck to false. I prefer the current approach because it is more representative of what happens in the real world, and still performs the global checks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 21, 2016
@Fishrock123

Fishrock123 commented Jul 21, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Sounds good to me.

We could probably get rid of turning off global checking in favor of always using the new API?

Edit, CI: https://ci.nodejs.org/job/node-test-pull-request/3374/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Jul 22, 2016

Copy link
Copy Markdown
Contributor Author

We could probably get rid of turning off global checking in favor of always using the new API?

Yep, that's the plan assuming this lands, and nothing too crazy is in the other tests.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Jul 22, 2016

Copy link
Copy Markdown
Contributor Author

CI was all 💚
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Aug 1, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@Fishrock123 can your "Sounds good to me." be taken as an official LGTM?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

@jasnell could I trouble you for a review?
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Aug 5, 2016

Copy link
Copy Markdown
Member

LGTM!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

Thanks, James! CI, since it's been a while: https://ci.nodejs.org/job/node-test-pull-request/3541/
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig
cjihrig force-pushed the globals branch from 0a3f56a to bbe5e17 Compare August 8, 2016 15:02
cjihrig added 2 commits August 8, 2016 11:03
@cjihrig
test: allow globals to be whitelisted
f18b1c9 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: nodejs#7826
Reviewed-By: James M Snell <[email protected]>
@cjihrig
repl: don't override all internal repl defaults
2d4a521 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: nodejs#7826
Reviewed-By: James M Snell <[email protected]>
@cjihrig
cjihrig force-pushed the globals branch from bbe5e17 to 2d4a521 Compare August 8, 2016 15:03
@cjihrig
cjihrig merged commit 2d4a521 into nodejs:master Aug 8, 2016
@cjihrig
cjihrig deleted the globals branch August 8, 2016 15:04
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
v6.4.0 Release Proposal #8020
Closed
cjihrig added a commit that referenced this pull request Aug 10, 2016
@cjihrig
test: allow globals to be whitelisted
9735acc 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: #7826
Reviewed-By: James M Snell <[email protected]>
cjihrig added a commit that referenced this pull request Aug 10, 2016
@cjihrig
repl: don't override all internal repl defaults
61e57e0 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: #7826
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
v6.4.0 Proposal #8070
Merged
@MylesBorins MylesBorins added the lts-watch-v4.x label Sep 30, 2016
@MylesBorins MylesBorins modified the milestones: v4.6.2, v4.7.0 Oct 24, 2016
@MylesBorins

MylesBorins commented Nov 18, 2016

Copy link
Copy Markdown
Contributor

ping @cjihrig
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig added a commit to cjihrig/node that referenced this pull request Nov 23, 2016
@cjihrig
test: allow globals to be whitelisted
fd97cf0 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: nodejs#7826
Reviewed-By: James M Snell <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this pull request Nov 23, 2016
@cjihrig
repl: don't override all internal repl defaults
9a0d9fc 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: nodejs#7826
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 23, 2016
Backport: repl: don't override all internal repl defaults #9774
Closed
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
@cjihrig @MylesBorins
test: allow globals to be whitelisted
02e8187 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: #7826
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
@cjihrig @MylesBorins
repl: don't override all internal repl defaults
698bf2e 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: #7826
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Dec 21, 2016
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@cjihrig @MylesBorins
test: allow globals to be whitelisted
2fd27a4 
This commit adds a function to test/common.js that allows
additional global variables to be whitelisted in a test.

PR-URL: #7826
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@cjihrig @MylesBorins
repl: don't override all internal repl defaults
13c4a6b 
The createInternalRepl() module accepts an options object as an
argument. However, if one is provided, it overrides all of the
default options. This commit applies the options object to the
defaults, only changing the values that are explicitly set.

PR-URL: #7826
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
V4.7.1 proposal #10395
Merged
MylesBorins added a commit that referenced this pull request Jan 3, 2017
@MylesBorins
2017-01-03, Version 4.7.1 'Argon' (LTS)
1035318 
This LTS release comes with 180 commits. This includes 117 which are
test related, 34 which are doc related, 15 which are build / tool
related, and 1 commit which is an update to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* repl:
  - Passing options to the repl will no longer overwrite defaults
    (cjihrig) #7826
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10395
MylesBorins added a commit that referenced this pull request Jan 4, 2017
@MylesBorins
2017-01-03, Version 4.7.1 'Argon' (LTS)
b26a469 
This LTS release comes with 180 commits. This includes 117 which are
test related, 34 which are doc related, 15 which are build / tool
related, and 1 commit which is an update to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* repl:
  - Passing options to the repl will no longer overwrite defaults
    (cjihrig) #7826
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10395
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
@imyller
2017-01-03, Version 4.7.1 'Argon' (LTS)
774acef 
    This LTS release comes with 180 commits. This includes 117 which are
    test related, 34 which are doc related, 15 which are build / tool
    related, and 1 commit which is an update to dependencies.

    Notable Changes:

    * build:
      - shared library support is now working for AIX builds
        (Stewart Addison) nodejs/node#9675
    * repl:
      - Passing options to the repl will no longer overwrite defaults
        (cjihrig) nodejs/node#7826
    * timers:
      - Re canceling a cancelled timers will no longer throw
        (Jeremiah Senkpiel) nodejs/node#9685

    PR-URL: nodejs/node#10395

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
@imyller
2017-01-03, Version 4.7.1 'Argon' (LTS)
13f539e 
    This LTS release comes with 180 commits. This includes 117 which are
    test related, 34 which are doc related, 15 which are build / tool
    related, and 1 commit which is an update to dependencies.

    Notable Changes:

    * build:
      - shared library support is now working for AIX builds
        (Stewart Addison) nodejs/node#9675
    * repl:
      - Passing options to the repl will no longer overwrite defaults
        (cjihrig) nodejs/node#7826
    * timers:
      - Re canceling a cancelled timers will no longer throw
        (Jeremiah Senkpiel) nodejs/node#9685

    PR-URL: nodejs/node#10395

Signed-off-by: Ilkka Myller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

4.7.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@cjihrig @Fishrock123 @jasnell @MylesBorins @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.