Skip to content

repl: fix require('3rdparty') regression #4215

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 9, 2015
Merged

repl: fix require('3rdparty') regression #4215

merged 1 commit into from
Dec 9, 2015

Conversation

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Dec 9, 2015

Fix module loading of third-party modules in the REPL by inheriting
module.paths from the REPL's parent module.

Commit ee72ee7 ("module,repl: remove repl require() hack") introduced
a regression where require() of modules in node_modules directories
no longer worked in the REPL (and fortunately only in the REPL.)
It turns out we didn't have test coverage for that but we do now.

Fixes: #4208

R=@silverwind?

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

@bnoordhuis bnoordhuis added the repl Issues and PRs related to the REPL subsystem. label Dec 9, 2015
@bnoordhuis bnoordhuis mentioned this pull request Dec 9, 2015
Closed
silverwind
silverwind reviewed Dec 9, 2015
View reviewed changes
test/parallel/test-repl-require.js Outdated
Copy link
Contributor

@silverwind silverwind Dec 9, 2015

Choose a reason for hiding this comment

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

Tiny nit: while answer gets hoisted, It'd be more clear if you move this above the server.listen block.

@silverwind
Copy link
Contributor

silverwind commented Dec 9, 2015

LGTM.

The repl/node_modules lookup I mentioned earlier is already present in <5.2 so i'm fine with keeping it, whatever its purpose might be :)

@silverwind
Copy link
Contributor

silverwind commented Dec 9, 2015

@nodejs/release we should follow up with a quick patch release once this has landed, as the bug made the REPL pretty much unuseable.

@bnoordhuis
repl: fix require('3rdparty') regression
213ede6 
Fix module loading of third-party modules in the REPL by inheriting
module.paths from the REPL's parent module.

Commit ee72ee7 ("module,repl: remove repl require() hack") introduced
a regression where require() of modules in node_modules directories
no longer worked in the REPL (and fortunately only in the REPL.)
It turns out we didn't have test coverage for that but we do now.

Fixes: nodejs#4208
PR-URL: nodejs#4215
Reviewed-By: Roman Reiss <me@silverwind.io>
@bnoordhuis bnoordhuis closed this Dec 9, 2015
@bnoordhuis bnoordhuis deleted the fix4208 branch December 9, 2015 22:31
@bnoordhuis bnoordhuis merged commit 213ede6 into nodejs:master Dec 9, 2015
@bnoordhuis
Copy link
Member Author

bnoordhuis commented Dec 9, 2015

Landed with the suggested style fix-up in 213ede6. Thanks for the review, @silverwind.

@bnoordhuis bnoordhuis mentioned this pull request Dec 9, 2015
Merged
@evanlucas evanlucas mentioned this pull request Dec 11, 2015
Closed
@cjihrig cjihrig mentioned this pull request Dec 12, 2015
Closed
This was referenced Dec 15, 2015
Closed
Merged
bnoordhuis added a commit that referenced this pull request Dec 15, 2015
@bnoordhuis @cjihrig
repl: fix require('3rdparty') regression
1999fdc 
Fix module loading of third-party modules in the REPL by inheriting
module.paths from the REPL's parent module.

Commit ee72ee7 ("module,repl: remove repl require() hack") introduced
a regression where require() of modules in node_modules directories
no longer worked in the REPL (and fortunately only in the REPL.)
It turns out we didn't have test coverage for that but we do now.

Fixes: #4208
PR-URL: #4215
Reviewed-By: Roman Reiss <me@silverwind.io>
cjihrig added a commit that referenced this pull request Dec 15, 2015
@cjihrig
2015-12-15, Version 5.3.0 (Stable)
ea5f3ab 
Notable changes:

* domains:
  - Fix handling of uncaught exceptions.
    (Julien Gilli) #3654.
* https:
  - Added support for disabling session caching.
    (Fedor Indutny) #4252.
* repl:
  - Allow third party modules to be imported using
    require(). This corrects a regression from 5.2.0.
    (Ben Noordhuis) #4215.
* deps:
  - Upgrade libuv to 1.8.0.
    (Saúl Ibarra Corretgé) #4276.

PR-URL: #4281
cjihrig added a commit that referenced this pull request Dec 16, 2015
@cjihrig
2015-12-16, Version 5.3.0 (Stable)
b21110f 
Notable changes:

* buffer:
  - Buffer.prototype.includes() has been added to keep parity
    with TypedArrays. (Alexander Martin) #3567.
* domains:
  - Fix handling of uncaught exceptions.
    (Julien Gilli) #3654.
* https:
  - Added support for disabling session caching.
    (Fedor Indutny) #4252.
* repl:
  - Allow third party modules to be imported using
    require(). This corrects a regression from 5.2.0.
    (Ben Noordhuis) #4215.
* deps:
  - Upgrade libuv to 1.8.0.
    (Saúl Ibarra Corretgé) #4276.

PR-URL: #4281
cjihrig added a commit that referenced this pull request Dec 16, 2015
@cjihrig
2015-12-16, Version 5.3.0 (Stable)
e5774c9 
Notable changes:

* buffer:
  - Buffer.prototype.includes() has been added to keep parity
    with TypedArrays. (Alexander Martin) #3567.
* domains:
  - Fix handling of uncaught exceptions.
    (Julien Gilli) #3654.
* https:
  - Added support for disabling session caching.
    (Fedor Indutny) #4252.
* repl:
  - Allow third party modules to be imported using
    require(). This corrects a regression from 5.2.0.
    (Ben Noordhuis) #4215.
* deps:
  - Upgrade libuv to 1.8.0.
    (Saúl Ibarra Corretgé) #4276.

PR-URL: #4281

Conflicts:
	src/node_version.h
@ljharb ljharb mentioned this pull request Dec 16, 2015
Closed
@rvagg rvagg mentioned this pull request Dec 17, 2015
Closed
@reqshark reqshark mentioned this pull request Dec 19, 2015
Closed
This was referenced Dec 29, 2015
Closed
Closed
cjihrig pushed a commit to cjihrig/node that referenced this pull request Jan 7, 2016
@bnoordhuis @cjihrig
repl: fix require('3rdparty') regression
82c9778 
Fix module loading of third-party modules in the REPL by inheriting
module.paths from the REPL's parent module.

Commit ee72ee7 ("module,repl: remove repl require() hack") introduced
a regression where require() of modules in node_modules directories
no longer worked in the REPL (and fortunately only in the REPL.)
It turns out we didn't have test coverage for that but we do now.

Fixes: nodejs#4208
PR-URL: nodejs#4215
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 7, 2016

@Fishrock123 I'm removing the land-on-v5.x label as it landed in v5.3.0

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jan 7, 2016

@thealphanerd I don't really pay attention to that anyways. The dont-land-on label(s) are much better for Stable branches since we basically just land everything that applies and isn't breaking and doesn't have that label.

MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
@bnoordhuis
repl: fix require('3rdparty') regression
41882e4 
Fix module loading of third-party modules in the REPL by inheriting
module.paths from the REPL's parent module.

Commit ee72ee7 ("module,repl: remove repl require() hack") introduced
a regression where require() of modules in node_modules directories
no longer worked in the REPL (and fortunately only in the REPL.)
It turns out we didn't have test coverage for that but we do now.

Fixes: #4208
PR-URL: #4215
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Merged
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@bnoordhuis
repl: fix require('3rdparty') regression
17d505d 
Fix module loading of third-party modules in the REPL by inheriting
module.paths from the REPL's parent module.

Commit ee72ee7 ("module,repl: remove repl require() hack") introduced
a regression where require() of modules in node_modules directories
no longer worked in the REPL (and fortunately only in the REPL.)
It turns out we didn't have test coverage for that but we do now.

Fixes: nodejs#4208
PR-URL: nodejs#4215
Reviewed-By: Roman Reiss <me@silverwind.io>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@cjihrig
2015-12-16, Version 5.3.0 (Stable)
c74c53a 
Notable changes:

* buffer:
  - Buffer.prototype.includes() has been added to keep parity
    with TypedArrays. (Alexander Martin) nodejs#3567.
* domains:
  - Fix handling of uncaught exceptions.
    (Julien Gilli) nodejs#3654.
* https:
  - Added support for disabling session caching.
    (Fedor Indutny) nodejs#4252.
* repl:
  - Allow third party modules to be imported using
    require(). This corrects a regression from 5.2.0.
    (Ben Noordhuis) nodejs#4215.
* deps:
  - Upgrade libuv to 1.8.0.
    (Saúl Ibarra Corretgé) nodejs#4276.

PR-URL: nodejs#4281

Conflicts:
	src/node_version.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bnoordhuis @silverwind @MylesBorins @Fishrock123