FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: fix require in node repl by ZYSzys · Pull Request #30835 · nodejs/node · GitHub

/ node Public

module: fix require in node repl - #30835

Closed
ZYSzys wants to merge 2 commits into
nodejs:masterfrom
zys-contrib:repl-require
ConversationCommits (2)ChecksFiles changed
Closed

module: fix require in node repl#30835
ZYSzys wants to merge 2 commits into
nodejs:masterfrom
zys-contrib:repl-require

Conversation

ZYSzys commented Dec 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes: #30808

In REPL, module.filename is null, and for relative path modules, we shouldn't look up relative modules from node_modules.

> module.filename
null
>
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

ZYSzys requested a review from bmeck December 7, 2019 13:24
ZYSzys added module Issues and PRs related to the module subsystem. repl Issues and PRs related to the REPL subsystem. labels Dec 7, 2019
ZYSzys removed the request for review from bmeck December 7, 2019 13:24

This comment has been minimized.

Sign in to view
ZYSzys commented Dec 7, 2019
View reviewed changes
Comment thread lib/internal/modules/cjs/loader.js
// 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 = ['.'];

ZYSzys Dec 7, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Sign in to view

nodejs-github-bot commented Dec 7, 2019

Copy link
Copy Markdown
Collaborator

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

ZYSzys mentioned this pull request Dec 7, 2019
Closed
BridgeAR approved these changes Dec 8, 2019
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
View reviewed changes

guybedford left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
View reviewed changes

guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Seems good. Missed that this code path is specifically for relative modules (seems I incorrectly expected that resolveLookupPaths would never even apply to relative paths in the first place).

nodejs-github-bot commented Dec 9, 2019

Copy link
Copy Markdown
Collaborator

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

ZYSzys added a commit that referenced this pull request Dec 10, 2019
module: fix require in node repl
7629fb2 
Fixes: #30808

PR-URL: #30835
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

ZYSzys commented Dec 10, 2019

Copy link
Copy Markdown
Member Author

Landed in 7629fb2.

ZYSzys closed this Dec 10, 2019
ZYSzys deleted the repl-require branch December 10, 2019 04:06
targos pushed a commit that referenced this pull request Dec 10, 2019
module: fix require in node repl
8e16093 
Fixes: #30808

PR-URL: #30835
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Dec 13, 2019
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
module: fix require in node repl
ffc910f 
Fixes: #30808

PR-URL: #30835
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
module: fix require in node repl
e5437ef 
Fixes: #30808

PR-URL: #30835
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Feb 8, 2020
Merged
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

guybedford guybedford approved these changes
Trott Trott approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

module Issues and PRs related to the module subsystem. repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Surprising require('./file') behavior when ran from REPL

5 participants

Back | FazBrowse Home | New Git URL