Skip to content

fix require.resolve not considering paths . and .. relative#56735

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
dario-piotrowicz:dario/require-resolve-opts-paths-dot-dot-fix
Jan 25, 2025
Merged

fix require.resolve not considering paths . and .. relative#56735
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
dario-piotrowicz:dario/require-resolve-opts-paths-dot-dot-fix

Conversation

@dario-piotrowicz
Copy link
Copy Markdown
Member

Fixes: #47000

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Jan 23, 2025
Comment thread lib/internal/modules/cjs/loader.js Outdated
@dario-piotrowicz dario-piotrowicz force-pushed the dario/require-resolve-opts-paths-dot-dot-fix branch from 9ffc34a to 7e988e5 Compare January 23, 2025 23:31
@dario-piotrowicz dario-piotrowicz changed the title src: fix require.resolve paths option not treating .. as relative fix require.resolve not considering paths . and .. relative Jan 23, 2025
@anonrig anonrig added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 23, 2025
@ljharb
Copy link
Copy Markdown
Member

ljharb commented Jan 23, 2025

do all of these added tests fail on main, or only a subset of them?

@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented Jan 24, 2025

The src: subsystem is reserved for changes in src/. Let's use module: instead

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2025
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@dario-piotrowicz
Copy link
Copy Markdown
Member Author

dario-piotrowicz commented Jan 24, 2025

do all of these added tests fail on main, or only a subset of them?

only a subset, but I think that the paths option is actually never tested anywhere else (or at least I think?) so I figured that adding some would be beneficial (without going overboard here)

Screenshot 2025-01-24 at 00 07 00

@dario-piotrowicz dario-piotrowicz force-pushed the dario/require-resolve-opts-paths-dot-dot-fix branch from c8424cc to 02cbc98 Compare January 24, 2025 00:09
@ljharb
Copy link
Copy Markdown
Member

ljharb commented Jan 24, 2025

ah ok, so this is specifically a bug with paths only?

@dario-piotrowicz
Copy link
Copy Markdown
Member Author

The src: subsystem is reserved for changes in src/. Let's use module: instead

sorry, my bad, updated ๐Ÿซก

@dario-piotrowicz
Copy link
Copy Markdown
Member Author

ah ok, so this is specifically a bug with paths only?

Yes, as far as I can tell that's the case, this for example:

// require(".") should resolve like require("./")
assert.strictEqual(a, b);

does indicate that . is correctly treated as a relative path in standard require calls

Comment thread test/parallel/test-require-resolve-opts-paths-relative.js
@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented Jan 24, 2025

May I suggest to not include add early return for perf gain and fix linting in the commit message โ€“ I would also suggest to not bother with the Fixes: which will be added by the bot upon landing as long as it's in the PR description.

The commit message needs to be shorter, e.g. module: fix relative path handling in `require.resolve` .

@dario-piotrowicz dario-piotrowicz force-pushed the dario/require-resolve-opts-paths-dot-dot-fix branch from 02cbc98 to 7a97c0b Compare January 24, 2025 00:20
@dario-piotrowicz
Copy link
Copy Markdown
Member Author

May I suggest to not include add early return for perf gain and fix linting in the commit message

Sorry I added them by mistake when I squashed fixup commits, I've already removed them

I would also suggest to not bother with the Fixes: which will be added by the bot upon landing as long as it's in the PR description.

Sorry the contributing MD made me thing I was supposed to add that ๐Ÿ˜“

The commit message needs to be shorter, e.g. module: fix relative path handling in `require.resolve`.

Yes I did notice the linting error, hopefully my newer title is ok?

@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented Jan 24, 2025

Sorry the contributing MD made me thing I was supposed to add that ๐Ÿ˜“

To be clear, you can 100% do that if you prefer to do it โ€“ what's important is for the commit landing on main to contain it, and what I'm saying is you can let the bot add it for you