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

module,repl: remove repl require() hack #4026

Merged
merged 2 commits into from
Nov 30, 2015

Conversation

bnoordhuis
Copy link
Member

I'll spruce up the commit logs once it's clear this is an approach we want to take.

R=@cjihrig

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

@bnoordhuis bnoordhuis added util Issues and PRs related to the built-in util module. module Issues and PRs related to the module subsystem. repl Issues and PRs related to the REPL subsystem. labels Nov 25, 2015
@Fishrock123
Copy link
Contributor

cc @chrisdickinson probably also

@cjihrig
Copy link
Contributor

cjihrig commented Nov 25, 2015

LGTM pending the CI

Remove a hack that was introduced in commit bb6d468 from November 2010.
This is groundwork for a follow-up commit that makes it possible to use
internal modules in lib/repl.js.

PR-URL: nodejs#4026
Reviewed-By: Colin Ihrig <[email protected]>
Move the method that was added in commit 8ca412b from earlier this month
from lib/util.js to lib/internal/util.js.

Avoids exposing a method that we may not wish to expose just yet, seeing
how it relies on implementation details.

PR-URL: nodejs#4026
Reviewed-By: Colin Ihrig <[email protected]>
@bnoordhuis bnoordhuis closed this Nov 30, 2015
@bnoordhuis bnoordhuis deleted the remove-repl-require-hack branch November 30, 2015 23:03
@bnoordhuis bnoordhuis merged commit 04b1a2f into nodejs:master Nov 30, 2015
@MylesBorins
Copy link
Contributor

should we land this on lts?

@cjihrig
Copy link
Contributor

cjihrig commented Dec 1, 2015

I think we should definitely make sure this is stable for a while since it involves the module system.

bnoordhuis added a commit that referenced this pull request Dec 5, 2015
Remove a hack that was introduced in commit bb6d468 from November 2010.
This is groundwork for a follow-up commit that makes it possible to use
internal modules in lib/repl.js.

PR-URL: #4026
Reviewed-By: Colin Ihrig <[email protected]>
bnoordhuis added a commit that referenced this pull request Dec 5, 2015
Move the method that was added in commit 8ca412b from earlier this month
from lib/util.js to lib/internal/util.js.

Avoids exposing a method that we may not wish to expose just yet, seeing
how it relies on implementation details.

PR-URL: #4026
Reviewed-By: Colin Ihrig <[email protected]>
@bnoordhuis
Copy link
Member Author

See bug report #4208 and bug fix #4215. If this PR gets landed in LTS, that PR should be included as well.

@rvagg rvagg mentioned this pull request Dec 17, 2015
cjihrig pushed a commit to cjihrig/node that referenced this pull request Jan 7, 2016
Remove a hack that was introduced in commit bb6d468 from November 2010.
This is groundwork for a follow-up commit that makes it possible to use
internal modules in lib/repl.js.

PR-URL: nodejs#4026
Reviewed-By: Colin Ihrig <[email protected]>

Conflicts:
	lib/module.js
cjihrig pushed a commit to cjihrig/node that referenced this pull request Jan 7, 2016
Move the method that was added in commit 8ca412b from earlier this month
from lib/util.js to lib/internal/util.js.

Avoids exposing a method that we may not wish to expose just yet, seeing
how it relies on implementation details.

PR-URL: nodejs#4026
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants