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

Self referential modules not supported in repl or -r #31595

Closed
MylesBorins opened this issue Jan 31, 2020 · 6 comments
Closed

Self referential modules not supported in repl or -r #31595

MylesBorins opened this issue Jan 31, 2020 · 6 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 31, 2020

If you attempt to require('package-name') using v13.7.0 in the repl it will fail with a 'MODULE_NOT_FOUND' error.

// index.js
console.log('yay')
// package.json
{
  "name": "ohno",
  "main": "./index.js"
}
$ node
> require('ohno')

Uncaught Error: Cannot find module 'ohno'
Require stack:

  • at Function.Module._resolveFilename (internal/modules/cjs/loader.js:980:15) at Function.Module._load (internal/modules/cjs/loader.js:862:27) at Module.require (internal/modules/cjs/loader.js:1040:19) at require (internal/modules/cjs/helpers.js:72:18) { code: 'MODULE_NOT_FOUND', requireStack: [ '' ]

}

$ node -r ./index.js
yay
$ node -r ohno

internal/modules/cjs/loader.js:983
throw err;
^

Error: Cannot find module 'ohno'
Require stack:

  • internal/preload
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:980:15)
    at Function.Module._load (internal/modules/cjs/loader.js:862:27)
    at Module.require (internal/modules/cjs/loader.js:1040:19)
    at Module._preloadModules (internal/modules/cjs/loader.js:1296:12)
    at loadPreloadModules (internal/bootstrap/pre_execution.js:435:5)
    at prepareMainThreadExecution (internal/bootstrap/pre_execution.js:68:3)
    at internal/main/repl.js:18:1 {
    code: 'MODULE_NOT_FOUND',
    requireStack: [ 'internal/preload' ]
    }
@MylesBorins MylesBorins added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. esm Issues and PRs related to the ECMAScript Modules implementation. labels Jan 31, 2020
@guybedford
Copy link
Contributor

guybedford commented Feb 1, 2020 via email

@dnlup
Copy link
Contributor

dnlup commented Mar 6, 2020

I would like to try to help on this issue. I am trying a few things and for what I have seen so far in the commonjs loader, at this line the self resolution is skipped because the parent module does not have a filename property.

@guybedford
Copy link
Contributor

@dnlup the parent.filename here should correspond to a index.js containing console.log(module.filename) giving the same filename as the parent in the place you've quoted. That is /path/to/index.js is the parent which is used to determine the context for the self resolution.

I just did a quick test of this with the following structure:

package.json

{ "name": "pkg", "exports": "./pkg.js" }

index.js

require('pkg');

pkg.js

console.log('it works');

and can confirm this is working ok, so will close this issue.

If you are still finding any inconsistencies though please do let me know - the fact that you also ran into this means that there may be some documentation gaps that we need to do a better job of filling where the catch is happening here.

@guybedford guybedford reopened this Mar 6, 2020
@guybedford
Copy link
Contributor

Sorry I misread this is specifically about calling from the repl.

In the case of the repl, strictly speaking can we consider the repl to be internal to a package I wonder?

@dnlup perhaps you can share the sort of use case you have in mind for using self resolution from the repl?

I could certainly get behind supporting this though - and yes it would be doing something like module.id === '<repl>' ? process.cwd() + path.sep : module.filename as the base.

@dnlup
Copy link
Contributor

dnlup commented Mar 6, 2020

Thanks @guybedford for the quick answer.

it would be doing something like module.id === '' ? process.cwd() + path.sep : module.filename as the base

Sounds good.

In the case of the repl, strictly speaking can we consider the repl to be internal to a package I wonder?

I would think so, that is how I am making tests right now.

perhaps you can share the sort of use case you have in mind for using self resolution from the repl?

As of now, it is just something that I think it could be done to have more feature parity with require outside the repl. Do not have a major use case, but I think it makes sense.

@guybedford guybedford added the feature request Issues that request new features to be added to Node.js. label Mar 12, 2020
@almosnow
Copy link

almosnow commented Jun 2, 2020

Is this done? Can I still help? :)

dnlup added a commit to dnlup/node that referenced this issue Jul 22, 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()`.

Refs:

* nodejs#31595
MylesBorins pushed a commit that referenced this issue 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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
guybedford pushed a commit to guybedford/node that referenced this issue 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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
codebytere pushed a commit that referenced this issue 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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants