module: accept Windows relative path #22186

joaocgreis · 2018-08-08T03:25:55Z

Before this change, require('.\\test.js') failed while require('./test.js') succeeded. This changes _resolveLookupPaths so that both ways are accepted on Windows.

Fixes: #21918 (cc @targos)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2018-08-08T03:25:56Z

@joaocgreis build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/456/pipeline

joaocgreis · 2018-08-08T03:32:13Z

lib/internal/modules/cjs/loader.js

-  if (parentIdPath === '.' && id.indexOf('/') === -1) {
+  if (parentIdPath === '.' &&
+      id.indexOf('/') === -1 &&
+      (!isWindows || id.indexOf('\\') === -1)) {
    id = './' + id;
  }


This change looked necessary, but this piece of code does not seem to be used anywhere in node core nor tested. newReturn seems to be passed as true everywhere. Hence, I'm not sure about this, let me know if I should leave this as it was.

newReturn is for backwards compatibility. There were some people using the private, underscored methods directly and were expecting the old return value.

Trott · 2018-08-09T18:08:22Z

test/parallel/test-module-relative-lookup.js

+runTest('./lodash');
+if (common.isWindows) {
+  runTest('.\\lodash');
+}


Should there be an else clause added that asserts that runTest('.\\lodash') on non-Windows throws or whatever it is supposed to do?

joaocgreis · 2018-08-10T18:46:44Z

@Trott thanks! Updated.

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/16346/

cc @nodejs/platform-windows. Not sure who to ping for modules (the modules-active-members team description points to esm), trying some previous commiters: @phillipj @silverwind

phillipj

LGTM

phillipj · 2018-08-15T07:40:11Z

Food for thought and too big a discussion for the changes done here, but I feel it's kinda awkward we allow POSIX require paths on Windows, but not Windows paths on POSIX. If we did, it would mean better code portability. But again keeping to POSIX paths would provide that portability cross Windows/POSIX already so 🤷‍♂️

joaocgreis · 2018-08-16T13:19:05Z

cc @nodejs/modules

I plan to land this next week if there are no objections.

bmeck · 2018-08-16T15:09:04Z

@joaocgreis This seems fine for require to diverge but I would note that import at least on the web will be expecting / to always be used; so, using \\ will likely remain fragile if we are to match how the web is doing resolution. I would not like to see this change occur for import, but this PR seems fine.

benjamingr

This is a huge change and effectively means that code developed on Windows has a lower chance of working on linux/mac which can cause divergence.

Module paths are not file paths.

I'm not sure I object to the change itself but I think there needs to be more discussion before we simply land this.

benjamingr · 2018-08-16T15:15:53Z

I would not like to see this change occur for import, but this PR seems fine.

@bmeck why would this be fine for require but not for esm?

Also cc @nodejs/platform-windows

benjamingr · 2018-08-16T15:17:11Z

I'd be totally fine with fixing #21918 though only for --require and not the general (in path) case since that is expected to be invoked from outside of Node.

bmeck · 2018-08-16T15:30:43Z

@benjamingr mostly because require is expected to work with filepath-like strings in my mental model and does not do input normalization to some standard format. This is just reinforcing the mental model to me. OS divergence was always part of my mental model for require at least, perhaps thats our difference?

jdalton · 2018-08-16T15:31:08Z

This PR is a small bug fix for cjs. I'm not sure I fully understand all the red x's to hold it.

@benjamingr are you objecting to the comment by @phillipj and not the PR?

Much of Node's code already allows forward and back slashes for Windows in paths. This PR fixes one place where it was missed.

benjamingr · 2018-08-16T15:36:56Z

@jdalton mostly, I did not want this to get merged without a LGTM from @nodejs/platform-windows since it changes the behaviour to diverge between windows and other platforms further.

If it's consistent and aligns with the policy - then sure.

jdalton · 2018-08-16T15:51:34Z

@benjamingr Ah ok. @joaocgreis is on the platform-windows team. \cc @bzoz for additional review.

Note: \cc @mhdawson I'd like to be added to @nodejs/platform-windows since its a neighboring Microsoft team (DevPlat) that's tasked with this and it has cross-over into my team (WebPlat DevX).

Clearing my review given clarifications.

Trott · 2018-08-16T22:57:21Z

_{@jdalton: I added you to platform-windows}

refack

Let's think about this a little bit more

refack · 2018-08-16T23:46:10Z

What does this produce:

console.log(require('.\\test.js') === require('./test.js'))

P.S. I'm tending towards -1

Much of Node's code already allows forward and back slashes for Windows in paths. This PR fixes one place where it was missed.

@jdalton I'm not sure that is a good enough argument to change CJS semantics... This has been there for the lifespan of CJS, right? I'd rather not change it.

Which brings up other windows FS quirks such as case sensitivity.

console.log(require('.\\test.js') === require('.\\Test.js'))

Or is that allowed while

console.log(require('./test.js') === require('./Test.js'))

is not...

jdalton · 2018-08-17T00:17:41Z

@refack
Currently, today (before this PR) this is how require works on Windows:

The example below assumes a path of C:\projects\demo\bar.js

foo.js

console.log("\n" + [
    path.resolve(".\\bar.js"),
    require(path.resolve(".\\bar.js")),
    require("./..\\demo\\bar.js"),
    require("..\\demo\\bar.js"),
    require("./bar.js")
].join("\n"))

bar.js

module.exports = "bar"

Running node .\\foo.js will log:

C:\projects\demo\bar.js
bar
bar
bar
bar

While I can do node .\\foo.js and require("..\\demo\\bar.js") and require("./..\\demo\\bar.js"), if I was to try to do require(".\\bar.js") from within foo.js it would throw a module not found error.

As a Windows user I've hit this issue on more than one occasion and was surprised it wasn't addressed. It motivated me fix in my own module loader (a year or so back). This inconsistency/oversight is one of the many cuts that makes the Node Windows experience less-great than other platforms.

refack · 2018-08-17T01:13:54Z

@jdalton so the bug is just with . prefixed IDs? .. works? ouch 🤕

I'd still like all of us to think about this, and would like more buy-in...
(plus a few more tests, esp. one that verifies that #21918 is fixed)

refack · 2018-08-17T01:14:43Z

BTW: isn't this semver-major?

ljharb · 2018-08-17T01:19:56Z

_{@jdalton would you be able to perhaps open a PR to https://github.com/browserify/resolve that adds a test case for this new behavior? :-D}

jdalton · 2018-08-17T02:02:02Z

@refack

so the bug is just with . prefixed IDs? .. works? ouch 🤕

Yep. Anywhere there is a one-off path separator check it's easy to overlook. One way to handled this is by abstracting the check to a helper method for broader internal use.

(plus a few more tests, esp. one that verifies that #21918 is fixed)

I dig more tests!

BTW: isn't this semver-major?

It can be.

@ljharb

_{would you be able to perhaps open a PR to https://github.com/browserify/resolve that adds a test case for this new behavior? :-D}

Yep!

benjamingr · 2018-08-17T14:16:32Z

Adding the semver-major tag per the discussion.

joaocgreis · 2018-08-22T21:33:05Z

I agree with semver-major. Before this, require('.\\test.js') could be used to ignore the current directory when looking for test.js.

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/16687/

Thanks for the reviews so far! Since this is now semver-major, two reviews from @nodejs/tsc are also required.

Trott · 2018-08-24T03:22:09Z

I rebased and tried to force push to fix the dreaded jinja LICENSE CRLF thing, but I don't have permissions to push. @joaocgreis: Can you rebase this against master?

refack · 2018-08-24T12:04:29Z

I rebased and tried to force push to fix the dreaded jinja LICENSE CRLF thing, but I don't have permissions to push. @joaocgreis: Can you rebase this against master?

Wait. If this reproduces the CRLF issue, I want to investigate.

joaocgreis · 2018-08-24T21:15:45Z

@Trott thanks! Rebased, here we go again: https://ci.nodejs.org/view/All/job/node-test-pull-request/16736/

@refack in case you need, the previous head was 9e10faa54ca78dea007b1b1997d92047c6512037, just the same 3 commits here on top of c6a54af. That was before #22340, the failures look as expected to me. Do you see something unexpected there?

joaocgreis · 2018-08-28T22:07:11Z

CI is green.

Trott

LGTM if CITGM doesn't find anything alarming.

Before this change, require('.\\test.js') failed while require('./test.js') succeeded. This changes _resolveLookupPaths so that both ways are accepted on Windows. Fixes: nodejs#21918

joaocgreis · 2018-09-02T00:44:09Z

CitGM (this PR): https://ci.nodejs.org/view/All/job/citgm-smoker/1523/
CitGM (master): https://ci.nodejs.org/view/All/job/citgm-smoker/1524/ (pending)

joaocgreis · 2018-09-05T18:57:45Z

@Trott CitGM looks quite red, but similar to master. Do you see anything alarming?

BridgeAR · 2018-09-05T20:31:33Z

@joaocgreis the failures all seem unrelated to me. These show up in all CITGM runs. There are still a couple of flakes.

BridgeAR · 2018-09-05T20:33:52Z

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

joaocgreis · 2018-09-12T02:24:38Z

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

joaocgreis · 2018-09-12T03:43:12Z

CI is all green.

Landed in b36c581.

Thanks for all reviews!

Before this change, require('.\\test.js') failed while require('./test.js') succeeded. This changes _resolveLookupPaths so that both ways are accepted on Windows. Fixes: #21918 PR-URL: #22186 Reviewed-By: Phillip Johnsen <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

joaocgreis added module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform. labels Aug 8, 2018

joaocgreis commented Aug 8, 2018

View reviewed changes

joaocgreis mentioned this pull request Aug 8, 2018

node --require fails with Windows path separator #21918

Closed

Trott reviewed Aug 9, 2018

View reviewed changes

phillipj approved these changes Aug 15, 2018

View reviewed changes

benjamingr previously requested changes Aug 16, 2018

View reviewed changes

jdalton approved these changes Aug 16, 2018

View reviewed changes

refack suggested changes Aug 16, 2018

View reviewed changes

benjamingr added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 17, 2018

refack approved these changes Aug 22, 2018

View reviewed changes

joaocgreis force-pushed the joaocgreis-I88-require-windows branch from 9e10faa to 98ce2d5 Compare August 24, 2018 21:06

joaocgreis added the tsc-review label Aug 28, 2018

joaocgreis requested a review from a team August 30, 2018 13:39

Trott approved these changes Sep 1, 2018

View reviewed changes

targos approved these changes Sep 1, 2018

View reviewed changes

joaocgreis removed the tsc-review label Sep 1, 2018

joaocgreis added 3 commits September 2, 2018 00:29

module: accept Windows relative path

cf0e7c2

Before this change, require('.\\test.js') failed while require('./test.js') succeeded. This changes _resolveLookupPaths so that both ways are accepted on Windows. Fixes: nodejs#21918

fixup: test windows path everywhere

1154edc

fixup: add explicit test for GH 21918

cf41cb0

joaocgreis force-pushed the joaocgreis-I88-require-windows branch from 98ce2d5 to cf41cb0 Compare September 1, 2018 23:29

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2018

joaocgreis closed this Sep 12, 2018

module: accept Windows relative path #22186

module: accept Windows relative path #22186

Conversation

joaocgreis commented Aug 8, 2018

Checklist

nodejs-github-bot commented Aug 8, 2018

joaocgreis Aug 8, 2018

Choose a reason for hiding this comment

mscdex Aug 8, 2018

Choose a reason for hiding this comment

Trott Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

joaocgreis commented Aug 10, 2018

phillipj left a comment

Choose a reason for hiding this comment

phillipj commented Aug 15, 2018

joaocgreis commented Aug 16, 2018

bmeck commented Aug 16, 2018

benjamingr left a comment

Choose a reason for hiding this comment

benjamingr commented Aug 16, 2018 • edited Loading

benjamingr commented Aug 16, 2018

bmeck commented Aug 16, 2018

jdalton commented Aug 16, 2018 • edited Loading

benjamingr commented Aug 16, 2018 • edited Loading

jdalton commented Aug 16, 2018

Trott commented Aug 16, 2018

refack left a comment

Choose a reason for hiding this comment

refack commented Aug 16, 2018 • edited Loading

jdalton commented Aug 17, 2018 • edited Loading

foo.js

bar.js

refack commented Aug 17, 2018

refack commented Aug 17, 2018

ljharb commented Aug 17, 2018

jdalton commented Aug 17, 2018

benjamingr commented Aug 17, 2018

joaocgreis commented Aug 22, 2018

Trott commented Aug 24, 2018

refack commented Aug 24, 2018

joaocgreis commented Aug 24, 2018

joaocgreis commented Aug 28, 2018

Trott left a comment

Choose a reason for hiding this comment

joaocgreis commented Sep 2, 2018

joaocgreis commented Sep 5, 2018

BridgeAR commented Sep 5, 2018

BridgeAR commented Sep 5, 2018

joaocgreis commented Sep 12, 2018

joaocgreis commented Sep 12, 2018

Trott Aug 9, 2018 •

edited

Loading

benjamingr commented Aug 16, 2018 •

edited

Loading

jdalton commented Aug 16, 2018 •

edited

Loading

benjamingr commented Aug 16, 2018 •

edited

Loading

refack commented Aug 16, 2018 •

edited

Loading

jdalton commented Aug 17, 2018 •

edited

Loading