module: self referential modules in repl or `-r`#32261

dnlup · 2020-03-14T15:26:30Z

Load self-referential modules from the repl and using the preload flag -r.
In both cases, the base path used for resolution is the current process.cwd().

Fix#31595

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

lib/internal/modules/cjs/loader.js

guybedford

Thanks for working on this @dnlup.

lib/internal/modules/cjs/loader.js

dnlup · 2020-03-30T06:43:10Z

@guybedford I added tests and removed the WIP. I hope I am not forgetting any test case.

lib/internal/modules/cjs/loader.js

test/parallel/test-repl-require-self-referential.js

guybedford

Thanks @dnlup - this seems good to me.

//cc @nodejs/modules-active-members

hybrist

Thanks for making this happen! I feel like this is reaching the limits of inlining the logic though.

lib/internal/modules/cjs/loader.js

dnlup · 2020-04-06T07:20:05Z

I tried to address all your suggestions, could you take another look?

hybrist

Thanks for your patience with this & sorry for the late review feedback. :) LGTM!

lib/internal/modules/cjs/loader.js

test/fixtures/self_ref_module/index.js

lib/internal/modules/cjs/loader.js

ljharb

LGTM!

lib/internal/modules/cjs/loader.js

dnlup · 2020-04-16T07:23:11Z

I have rebased against master

nodejs-github-bot · 2020-04-17T22:29:56Z

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

dnlup · 2020-06-09T06:50:46Z

rebased the latest changes from master

nodejs-github-bot · 2020-06-21T07:14:29Z

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

dnlup · 2020-07-02T06:53:03Z

Should I keep this PR open?

ljharb · 2020-07-02T16:22:14Z

It seems like the only barrier to it landing is that CI is failing?

MylesBorins · 2020-07-02T21:20:00Z

I'm going on vacation for a week, but can help with bringing this over the finish line when I get back.

dnlup · 2020-07-03T07:45:43Z

Only the windows build seems to be failing, I don't understand why though, I can't find a specific error message beside the report that the test exited with code 1.

richardlau · 2020-07-03T11:20:34Z

Only the windows build seems to be failing, I don't understand why though, I can't find a specific error message beside the report that the test exited with code 1.

It's #34163. The GitHub actions UI is not good for searching through the logs. I find it easier to search the raw log:

dnlup · 2020-07-03T12:34:24Z

Thank you @richardlau

dnlup · 2020-07-17T07:15:22Z

After rebasing to resolve the conflicts I am getting ERR_INVALID_MODULE_SPECIFIER in my tests. 🙏 Give me some time to investigate this. It looks like this check wasn't there before, there must be something new that I have missed.

lib/internal/modules/esm/resolve.js

guybedford

Just noted one more refactoring here, sorry to be difficult but would just be nice to try keep the arguments primitive for functions where we can.

Otherwise lets get this merged now.

lib/internal/modules/cjs/loader.js

Load self referential modules from the repl and using the preload flag `-r`. In both cases the base path used for resolution is the current `process.cwd()`. Refs: * nodejs#31595

guybedford

Let's get this landed finally...

nodejs-github-bot · 2020-07-23T00:16:50Z

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

Load self referential modules from the repl and using the preload flag `-r`. In both cases the base path used for resolution is the current `process.cwd()`. Also fixes an internal cycle bug in the REPL exports resolution. PR-URL: #32261Fixes: #31595 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Jan Krems <[email protected]>

guybedford · 2020-07-23T14:50:53Z

Landed in ec2ffd6. I also marked this as a bug and regression since it fixes a real "exports" resolution regression in the REPL now. Thanks @dnlup for your persistence here.

dnlup · 2020-07-23T16:01:58Z

Thank you all for the help with this PR.

Load self referential modules from the repl and using the preload flag `-r`. In both cases the base path used for resolution is the current `process.cwd()`. Also fixes an internal cycle bug in the REPL exports resolution. PR-URL: #32261Fixes: #31595 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Jan Krems <[email protected]>

Load self referential modules from the repl and using the preload flag `-r`. In both cases the base path used for resolution is the current `process.cwd()`. Also fixes an internal cycle bug in the REPL exports resolution. PR-URL: nodejs#32261Fixes: nodejs#31595 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Jan Krems <[email protected]>

Load self referential modules from the repl and using the preload flag `-r`. In both cases the base path used for resolution is the current `process.cwd()`. Also fixes an internal cycle bug in the REPL exports resolution. PR-URL: #32261 Backport-PR-URL: #35385Fixes: #31595 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Jan Krems <[email protected]>

dnlup force-pushed the self_referential branch from 58b679c to ae72ed3Compare March 14, 2020 15:27

dnlup commented Mar 14, 2020
View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved

dnlup force-pushed the self_referential branch from ae72ed3 to 54ffeaaCompare March 14, 2020 15:38

dnlup changed the title ~~[WIP] module: self referential modules in repl or -r~~module: self referential modules in repl or -rMar 15, 2020

guybedford reviewed Mar 15, 2020
View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved

dnlup changed the title ~~module: self referential modules in repl or -r~~[WIP] module: self referential modules in repl or -rMar 24, 2020

dnlup changed the title ~~[WIP] module: self referential modules in repl or -r~~module: self referential modules in repl or -rMar 30, 2020

guybedford reviewed Mar 30, 2020
View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved
test/parallel/test-repl-require-self-referential.js Outdated Show resolvedHide resolved

guybedford approved these changes Mar 31, 2020
View reviewed changes

hybrist reviewed Mar 31, 2020
View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved

dnlup force-pushed the self_referential branch from 9684e99 to fc9022bCompare April 6, 2020 07:13

hybrist approved these changes Apr 6, 2020
View reviewed changes

ljharb reviewed Apr 6, 2020
View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved
test/fixtures/self_ref_module/index.js Outdated Show resolvedHide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved

ljharb reviewed Apr 8, 2020
View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved

dnlup force-pushed the self_referential branch from 43257ad to 86749a2Compare April 16, 2020 07:22

dnlup force-pushed the self_referential branch from 86749a2 to 8491203Compare May 19, 2020 07:06

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72Compare May 31, 2020 12:18

dnlup force-pushed the self_referential branch from 8491203 to e5938efCompare June 9, 2020 06:49

dnlup force-pushed the self_referential branch from e5938ef to c46e315Compare July 2, 2020 06:50

MylesBorins self-assigned this Jul 2, 2020

dnlup force-pushed the self_referential branch from c46e315 to 512004eCompare July 3, 2020 05:54

dnlup force-pushed the self_referential branch from 512004e to 07c2b68Compare July 16, 2020 07:29

dnlup force-pushed the self_referential branch 2 times, most recently from 3c0389b to 8618148Compare July 20, 2020 06:23

dnlup commented Jul 20, 2020
View reviewed changes

lib/internal/modules/esm/resolve.js Outdated Show resolvedHide resolved

dnlup force-pushed the self_referential branch from 8618148 to 9c0c66eCompare July 22, 2020 05:28

guybedford approved these changes Jul 22, 2020
View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolvedHide resolved

module: self referential modules in repl or -r
03c633f
Load self referential modules from the repl and using the preload flag `-r`. In both cases the base path used for resolution is the current `process.cwd()`. Refs: * nodejs#31595

dnlup force-pushed the self_referential branch from 9c0c66e to 03c633fCompare July 22, 2020 12:47

guybedford approved these changes Jul 23, 2020
View reviewed changes

guybedford added confirmed-bug Issues with confirmed bugs. regression Issues related to regressions. module Issues and PRs related to the module subsystem. labels Jul 23, 2020

guybedford closed this Jul 23, 2020

ruyadorno mentioned this pull request Jul 28, 2020
v14.7.0 proposal #34542
Merged

ZYSzys mentioned this pull request Aug 3, 2020
REPL throws on requiring a package with exports field #34521
Closed

addaleax added the backport-open-v12.x label Sep 28, 2020

codebytere mentioned this pull request Oct 4, 2020
v12.19.0 proposal #35401
Merged

mhdawson mentioned this pull request Jan 18, 2022
pummel/test-worker-take-heapsnapshot - crash in debug mode on arm #41204
Closed

Uh oh!

module: self referential modules in repl or -r#32261

module: self referential modules in repl or -r#32261

Uh oh!

Conversation

dnlup commented Mar 14, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dnlup commented Mar 30, 2020

Uh oh!

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

hybrist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnlup commented Apr 6, 2020

Uh oh!

hybrist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dnlup commented Apr 16, 2020

Uh oh!

nodejs-github-bot commented Apr 17, 2020

Uh oh!

dnlup commented Jun 9, 2020

Uh oh!

nodejs-github-bot commented Jun 21, 2020

Uh oh!

dnlup commented Jul 2, 2020

Uh oh!

ljharb commented Jul 2, 2020

Uh oh!

MylesBorins commented Jul 2, 2020

Uh oh!

dnlup commented Jul 3, 2020

Uh oh!

richardlau commented Jul 3, 2020

Uh oh!

dnlup commented Jul 3, 2020

Uh oh!

dnlup commented Jul 17, 2020

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 23, 2020

Uh oh!

guybedford commented Jul 23, 2020

Uh oh!

dnlup commented Jul 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

module: self referential modules in repl or `-r`#32261

module: self referential modules in repl or `-r`#32261

dnlup commented Mar 14, 2020•
edited
Loading