module: fix require in node repl #30835
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZYSzys
added
module
This comment has been minimized.
This comment has been minimized.
ZYSzys
commented
Dec 7, 2019
| // from realpath(__filename) but with eval there is no filename | ||
| const mainPaths = ['.'].concat(Module._nodeModulePaths('.'), modulePaths); | ||
| // from realpath(__filename) but in REPL there is no filename | ||
| const mainPaths = ['.']; |
There was a problem hiding this comment.
With --eval, parent has both id and filename set now, so I think this edge case wasn't for --eval anymore but for REPL mode.
> node --eval 'console.log(module)'
Module {
id: '[eval]',
path: '.',
exports: {},
parent: undefined,
filename: '/Users/zyszys/Projects/nodejs/node/[eval]',
loaded: false,
children: [],
paths: [
'/Users/zyszys/Projects/nodejs/node/node_modules',
'/Users/zyszys/Projects/nodejs/node_modules',
'/Users/zyszys/Projects/node_modules',
'/Users/zyszys/node_modules',
'/Users/node_modules',
'/node_modules'
]
}
This comment has been minimized.
This comment has been minimized.
BridgeAR
approved these changes
Dec 8, 2019
There was a problem hiding this comment.
This does seem correct. It would still be good to get another review from e.g. @devsnek or @guybedford though.
guybedford
reviewed
Dec 9, 2019
There was a problem hiding this comment.
Can you confirm that node --eval 'require("pkg")' where the file system looks like:
/path/to/cwd/
/path/to/node_modules/pkg/index.js
and the node process is running in the /path/to/cwd/ folder?
If we are cutting off those node_modules lookups that seems odd to me.
Edit: Misunderstood this was a relative code path, all seems good.
guybedford
approved these changes
Dec 9, 2019
Trott
approved these changes
Dec 9, 2019
ZYSzys
added a commit
that referenced
this pull request
Dec 10, 2019
Fixes: #30808 PR-URL: #30835 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Rich Trott <[email protected]>
|
Landed in 7629fb2. |
targos
pushed a commit
that referenced
this pull request
Dec 10, 2019
Fixes: #30808 PR-URL: #30835 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Merged
targos
pushed a commit
that referenced
this pull request
Jan 14, 2020
Fixes: #30808 PR-URL: #30835 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Rich Trott <[email protected]>
BethGriggs
pushed a commit
that referenced
this pull request
Feb 6, 2020
Fixes: #30808 PR-URL: #30835 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #30808
In REPL,
module.filenameisnull, and for relative path modules, we shouldn't look up relative modules fromnode_modules.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes