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: self referential modules in repl or -r #32261

Closed
wants to merge 1 commit into from

Conversation

dnlup
Copy link
Contributor

@dnlup dnlup commented Mar 14, 2020

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

@dnlup dnlup changed the title [WIP] module: self referential modules in repl or -r module: self referential modules in repl or -r Mar 15, 2020
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @dnlup.

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
@dnlup dnlup changed the title module: self referential modules in repl or -r [WIP] module: self referential modules in repl or -r Mar 24, 2020
@dnlup dnlup changed the title [WIP] module: self referential modules in repl or -r module: self referential modules in repl or -r Mar 30, 2020
@dnlup
Copy link
Contributor Author

dnlup commented Mar 30, 2020

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

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Thanks @dnlup - this seems good to me.

//cc @nodejs/modules-active-members

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

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

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
@dnlup
Copy link
Contributor Author

dnlup commented Apr 6, 2020

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

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

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

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
test/fixtures/self_ref_module/index.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM!

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Show resolved Hide resolved
@dnlup
Copy link
Contributor Author

dnlup commented Apr 16, 2020

I have rebased against master

@nodejs-github-bot
Copy link
Collaborator

@dnlup
Copy link
Contributor Author

dnlup commented Jun 9, 2020

rebased the latest changes from master

@nodejs-github-bot
Copy link
Collaborator

@dnlup
Copy link
Contributor Author

dnlup commented Jul 2, 2020

Should I keep this PR open?

@ljharb
Copy link
Member

ljharb commented Jul 2, 2020

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

@MylesBorins
Copy link
Contributor

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

@MylesBorins MylesBorins self-assigned this Jul 2, 2020
@dnlup
Copy link
Contributor Author

dnlup commented Jul 3, 2020

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
Copy link
Member

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:

image

@dnlup
Copy link
Contributor Author

dnlup commented Jul 3, 2020

Thank you @richardlau

@dnlup
Copy link
Contributor Author

dnlup commented Jul 17, 2020

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.

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

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
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
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Let's get this landed finally...

@nodejs-github-bot
Copy link
Collaborator

@guybedford 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 pushed a commit that referenced this pull request Jul 23, 2020
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
Fixes: #31595
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@guybedford
Copy link
Contributor

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.

@guybedford guybedford closed this Jul 23, 2020
@dnlup
Copy link
Contributor Author

dnlup commented Jul 23, 2020

Thank you all for the help with this PR.

MylesBorins pushed a commit that referenced this pull request Jul 27, 2020
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
Fixes: #31595
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Jul 28, 2020
guybedford pushed a commit to guybedford/node that referenced this pull request Sep 28, 2020
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#32261
Fixes: nodejs#31595
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
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: #35385
Fixes: #31595
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@codebytere codebytere mentioned this pull request Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. module Issues and PRs related to the module subsystem. regression Issues related to regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Self referential modules not supported in repl or -r
8 participants