module: revert checking for package.json before extensions

[bmeck]

This PR reverts a regression from a change in behavior that deviated from the documented behavior of the module resolution algorithm. The deviation caused the module resolution algorithm to check for package.json files when traversing up to a parent directory before following the documented module resolution algorithm.

Link discussing the regression in greater depth

Checklist

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

Affected core subsystem(s)

module

[ljharb]

would love this + a backport

[mscdex]

I'm not sure we need to use the word "revert" if the current behavior was added a very long time ago.

[bmeck]

@mscdex it has accidental side effects, I would prefer to keep that word.

[ljharb]

If it was unintentional and undocumented, it's a bug - I think the term isn't bound by recency.

[targos]

/cc @nodejs/tsc

I am in favor of this change

[BridgeAR]

I am also in favor of this. It seems like a more correct approach than the way it is currently working.

As there was no progress for a long time, I would like to have the @nodejs/tsc to have a look at this and to decide on how to move forward. There are already two TSC votes in favor.

[BridgeAR]

This needs a rebase. LGTM otherwise

[BridgeAR]

There is a new eslint rule in place that prevents messages without dynamic content (or it is at least planned). I do see that it makes sense to have a individual message here, so we should either explicitly accept the message here or maybe just change the inner.js export content to contain a property with a string, remove the message and use deepStrictEqual. With the latter the error message should be explicit enough.

[Trott]

@BridgeAR If that rule ever comes to pass, we can always disable it with a comment for the instances where a string literal makes sense. So this can stay as is, if the proposed lint rule is the only motivation for changing it.

[BridgeAR]

@bmeck I guess you actually want this to be looked at and it to get merged, right?

In that case it will need a rebase and the title should be changed accordingly.

[fhinkel]

@BridgeAR I don't see any -1. With the approvals, I think we can go ahead and merge it (after a rebase). Can we take this off the TSC agenda? Or am I missing something?

[mcollina]

LGTM with a better PR description and commit description

[BridgeAR]

@fhinkel sure, I just removed the labels. I just wanted to make sure it is properly looked at as it was originally controversial if that should be changed or not.

[BridgeAR]

Ping @bmeck

[bmeck]

I can make a better description tomorrow and I think we are approved beyond that.

[jasnell]

just double checking... this will land in v10

Should be plenty of time for that assuming it lands by end of March... :-)

[jdalton]

Anyone know about the treatment of the trailing /. In some places I see logic to handle \(for Windows). I'm wondering if it's needed here too.

Update:

Okay confirmed. Windows supports paths ending in \. and \... I think the check should be done to support both or branch per platform as is common elsewhere in Node.

[bmeck]

@jdalton

I think the check should be done to support both or branch per platform as is common elsewhere in Node.

The surrounding code is only looking at / characters outside of this PR, it looks like \\ has been swapped by the point it gets to that otherwise there would be lots of bugs in module.js.

[jdalton]

It looks like it goes

`require` then
`Module.prototype.require` then
`Module._resolveFilename` then
`Module._findPath`, with array from `Module._resolveLookupPaths`, then
// this code

The request isn't normalized through those passes yet.
The bug is likely there with the initial trailingSlash check in Node today too.

Update:

Filed the bug: #18299.

[jdalton]

👆 at the moment because the trailing slash check is for forward-slash-only, on Windows paths ending in a backslash hit that code path (which is removed in this PR). This means it will now fall through to the tryExtensions call on line 234, instead of the tryPackage call on line 243.

Note: Assumes a fixed #18299

[BridgeAR]

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

[BridgeAR]

@bmeck it seems like there is a lint error and a tests constantly fails:

not ok 1376 parallel/test-require-extensions-same-filename-as-dir
---
duration_ms: 0.171
severity: fail
stack: |-
assert.js:74
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: undefined strictEqual 'artischocko'
at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-require-extensions-same-filename-as-dir.js:31:8)
at Module._compile (module.js:668:30)
at Object.Module._extensions..js (module.js:679:10)
at Module.load (module.js:580:32)
at tryModuleLoad (module.js:520:12)
at Function.Module._load (module.js:512:3)
at Function.Module.runMain (module.js:709:10)
at startup (bootstrap_node.js:189:16)
at bootstrap_node.js:699:3

Would you be so kind and take a look? :-)

[BridgeAR]

I just rebased this, fixed the linter error and fixed the test issue. I am a bit confused that the old test is now working as before.

@bmeck can you please have a look at this?

[BridgeAR]

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

[bmeck]

@BridgeAR I'm dealing w/ family, won't get to this in next few days.

[BridgeAR]

Ping @bmeck. I hope everything is fine on your side :-)

[bmeck]

@BridgeAR I'm still pretty under water with workload due to things, nothing seems really apparent on why it would fail. I could try to revert and then git bisect if desired but won't get to it for a while.

[BridgeAR]

@bmeck I fixed the failure in 39f51aa but I wanted you to confirm that this is the right thing. To me it seems like a independent change made it possible to have this change without having a impact on any existing code.

[BridgeAR]

And I rebased and pushed the changes here. So this is good to go if you give your LG.

[bmeck]

@BridgeAR that test change looks correct, this PR should be adding trailing / effectively when going up a directory. So it should preserve upwards traversal looking at package.json with that test which uses ../.. for the require specifier. It would have had to have been ../../../module-stub for it to grab module-stub.json instead.

[BridgeAR]

Landed in 1ed36ae 🎉

[bmeck]

This PR reverts a regression from a change in behavior that deviated from the documented behavior of the module resolution algorithm. The deviation caused the module resolution algorithm to check for package.json files when traversing up to a parent directory before following the documented module resolution algorithm.

Link discussing the regression in greater depth

Checklist

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

Affected core subsystem(s)

module

[ljharb]

would love this + a backport

[mscdex]

I'm not sure we need to use the word "revert" if the current behavior was added a very long time ago.

[bmeck]

@mscdex it has accidental side effects, I would prefer to keep that word.

[ljharb]

If it was unintentional and undocumented, it's a bug - I think the term isn't bound by recency.

[targos]

/cc @nodejs/tsc

I am in favor of this change

[BridgeAR]

I am also in favor of this. It seems like a more correct approach than the way it is currently working.

As there was no progress for a long time, I would like to have the @nodejs/tsc to have a look at this and to decide on how to move forward. There are already two TSC votes in favor.

[BridgeAR]

This needs a rebase. LGTM otherwise

[BridgeAR]

There is a new eslint rule in place that prevents messages without dynamic content (or it is at least planned). I do see that it makes sense to have a individual message here, so we should either explicitly accept the message here or maybe just change the inner.js export content to contain a property with a string, remove the message and use deepStrictEqual. With the latter the error message should be explicit enough.

[Trott]

@BridgeAR If that rule ever comes to pass, we can always disable it with a comment for the instances where a string literal makes sense. So this can stay as is, if the proposed lint rule is the only motivation for changing it.

[BridgeAR]

@bmeck I guess you actually want this to be looked at and it to get merged, right?

In that case it will need a rebase and the title should be changed accordingly.

[fhinkel]

@BridgeAR I don't see any -1. With the approvals, I think we can go ahead and merge it (after a rebase). Can we take this off the TSC agenda? Or am I missing something?

[mcollina]

LGTM with a better PR description and commit description

[BridgeAR]

@fhinkel sure, I just removed the labels. I just wanted to make sure it is properly looked at as it was originally controversial if that should be changed or not.

[BridgeAR]

Ping @bmeck

[bmeck]

I can make a better description tomorrow and I think we are approved beyond that.

[jasnell]

just double checking... this will land in v10

Should be plenty of time for that assuming it lands by end of March... :-)

[jdalton]

Anyone know about the treatment of the trailing /. In some places I see logic to handle \(for Windows). I'm wondering if it's needed here too.

Update:

Okay confirmed. Windows supports paths ending in \. and \... I think the check should be done to support both or branch per platform as is common elsewhere in Node.

[bmeck]

@jdalton

I think the check should be done to support both or branch per platform as is common elsewhere in Node.

The surrounding code is only looking at / characters outside of this PR, it looks like \\ has been swapped by the point it gets to that otherwise there would be lots of bugs in module.js.

[jdalton]

It looks like it goes

`require` then
`Module.prototype.require` then
`Module._resolveFilename` then
`Module._findPath`, with array from `Module._resolveLookupPaths`, then
// this code

The request isn't normalized through those passes yet.
The bug is likely there with the initial trailingSlash check in Node today too.

Update:

Filed the bug: #18299.

[jdalton]

👆 at the moment because the trailing slash check is for forward-slash-only, on Windows paths ending in a backslash hit that code path (which is removed in this PR). This means it will now fall through to the tryExtensions call on line 234, instead of the tryPackage call on line 243.

Note: Assumes a fixed #18299

[BridgeAR]

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

[BridgeAR]

@bmeck it seems like there is a lint error and a tests constantly fails:

not ok 1376 parallel/test-require-extensions-same-filename-as-dir
---
duration_ms: 0.171
severity: fail
stack: |-
assert.js:74
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: undefined strictEqual 'artischocko'
at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-require-extensions-same-filename-as-dir.js:31:8)
at Module._compile (module.js:668:30)
at Object.Module._extensions..js (module.js:679:10)
at Module.load (module.js:580:32)
at tryModuleLoad (module.js:520:12)
at Function.Module._load (module.js:512:3)
at Function.Module.runMain (module.js:709:10)
at startup (bootstrap_node.js:189:16)
at bootstrap_node.js:699:3

Would you be so kind and take a look? :-)

[BridgeAR]

I just rebased this, fixed the linter error and fixed the test issue. I am a bit confused that the old test is now working as before.

@bmeck can you please have a look at this?

[BridgeAR]

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

[bmeck]

@BridgeAR I'm dealing w/ family, won't get to this in next few days.

[BridgeAR]

Ping @bmeck. I hope everything is fine on your side :-)

[bmeck]

@BridgeAR I'm still pretty under water with workload due to things, nothing seems really apparent on why it would fail. I could try to revert and then git bisect if desired but won't get to it for a while.

[BridgeAR]

@bmeck I fixed the failure in 39f51aa but I wanted you to confirm that this is the right thing. To me it seems like a independent change made it possible to have this change without having a impact on any existing code.

[BridgeAR]

And I rebased and pushed the changes here. So this is good to go if you give your LG.

[bmeck]

@BridgeAR that test change looks correct, this PR should be adding trailing / effectively when going up a directory. So it should preserve upwards traversal looking at package.json with that test which uses ../.. for the require specifier. It would have had to have been ../../../module-stub for it to grab module-stub.json instead.

[BridgeAR]

Landed in 1ed36ae 🎉

[bmeck]

This PR reverts a regression from a change in behavior that deviated from the documented behavior of the module resolution algorithm. The deviation caused the module resolution algorithm to check for package.json files when traversing up to a parent directory before following the documented module resolution algorithm.

Link discussing the regression in greater depth

Checklist

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

Affected core subsystem(s)

module

[ljharb]

would love this + a backport

[mscdex]

I'm not sure we need to use the word "revert" if the current behavior was added a very long time ago.

[bmeck]

@mscdex it has accidental side effects, I would prefer to keep that word.

[ljharb]

If it was unintentional and undocumented, it's a bug - I think the term isn't bound by recency.

[targos]

/cc @nodejs/tsc

I am in favor of this change

[BridgeAR]

I am also in favor of this. It seems like a more correct approach than the way it is currently working.

As there was no progress for a long time, I would like to have the @nodejs/tsc to have a look at this and to decide on how to move forward. There are already two TSC votes in favor.

[BridgeAR]

This needs a rebase. LGTM otherwise

[BridgeAR]

There is a new eslint rule in place that prevents messages without dynamic content (or it is at least planned). I do see that it makes sense to have a individual message here, so we should either explicitly accept the message here or maybe just change the inner.js export content to contain a property with a string, remove the message and use deepStrictEqual. With the latter the error message should be explicit enough.

[Trott]

@BridgeAR If that rule ever comes to pass, we can always disable it with a comment for the instances where a string literal makes sense. So this can stay as is, if the proposed lint rule is the only motivation for changing it.

[BridgeAR]

@bmeck I guess you actually want this to be looked at and it to get merged, right?

In that case it will need a rebase and the title should be changed accordingly.

[fhinkel]

@BridgeAR I don't see any -1. With the approvals, I think we can go ahead and merge it (after a rebase). Can we take this off the TSC agenda? Or am I missing something?

[mcollina]

LGTM with a better PR description and commit description

[BridgeAR]

@fhinkel sure, I just removed the labels. I just wanted to make sure it is properly looked at as it was originally controversial if that should be changed or not.

[BridgeAR]

Ping @bmeck

[bmeck]

I can make a better description tomorrow and I think we are approved beyond that.

[jasnell]

just double checking... this will land in v10

Should be plenty of time for that assuming it lands by end of March... :-)

[jdalton]

Anyone know about the treatment of the trailing /. In some places I see logic to handle \(for Windows). I'm wondering if it's needed here too.

Update:

Okay confirmed. Windows supports paths ending in \. and \... I think the check should be done to support both or branch per platform as is common elsewhere in Node.

[bmeck]

@jdalton

I think the check should be done to support both or branch per platform as is common elsewhere in Node.

The surrounding code is only looking at / characters outside of this PR, it looks like \\ has been swapped by the point it gets to that otherwise there would be lots of bugs in module.js.

[jdalton]

It looks like it goes

`require` then
`Module.prototype.require` then
`Module._resolveFilename` then
`Module._findPath`, with array from `Module._resolveLookupPaths`, then
// this code

The request isn't normalized through those passes yet.
The bug is likely there with the initial trailingSlash check in Node today too.

Update:

Filed the bug: #18299.

[jdalton]

👆 at the moment because the trailing slash check is for forward-slash-only, on Windows paths ending in a backslash hit that code path (which is removed in this PR). This means it will now fall through to the tryExtensions call on line 234, instead of the tryPackage call on line 243.

Note: Assumes a fixed #18299

[BridgeAR]

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

[BridgeAR]

@bmeck it seems like there is a lint error and a tests constantly fails:

not ok 1376 parallel/test-require-extensions-same-filename-as-dir
---
duration_ms: 0.171
severity: fail
stack: |-
assert.js:74
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: undefined strictEqual 'artischocko'
at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-require-extensions-same-filename-as-dir.js:31:8)
at Module._compile (module.js:668:30)
at Object.Module._extensions..js (module.js:679:10)
at Module.load (module.js:580:32)
at tryModuleLoad (module.js:520:12)
at Function.Module._load (module.js:512:3)
at Function.Module.runMain (module.js:709:10)
at startup (bootstrap_node.js:189:16)
at bootstrap_node.js:699:3

Would you be so kind and take a look? :-)

[BridgeAR]

I just rebased this, fixed the linter error and fixed the test issue. I am a bit confused that the old test is now working as before.

@bmeck can you please have a look at this?

[BridgeAR]

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

[bmeck]

@BridgeAR I'm dealing w/ family, won't get to this in next few days.

[BridgeAR]

Ping @bmeck. I hope everything is fine on your side :-)

[bmeck]

@BridgeAR I'm still pretty under water with workload due to things, nothing seems really apparent on why it would fail. I could try to revert and then git bisect if desired but won't get to it for a while.

[BridgeAR]

@bmeck I fixed the failure in 39f51aa but I wanted you to confirm that this is the right thing. To me it seems like a independent change made it possible to have this change without having a impact on any existing code.

[BridgeAR]

And I rebased and pushed the changes here. So this is good to go if you give your LG.

[bmeck]

@BridgeAR that test change looks correct, this PR should be adding trailing / effectively when going up a directory. So it should preserve upwards traversal looking at package.json with that test which uses ../.. for the require specifier. It would have had to have been ../../../module-stub for it to grab module-stub.json instead.

[BridgeAR]

Landed in 1ed36ae 🎉

[bmeck]

This PR reverts a regression from a change in behavior that deviated from the documented behavior of the module resolution algorithm. The deviation caused the module resolution algorithm to check for package.json files when traversing up to a parent directory before following the documented module resolution algorithm.

Link discussing the regression in greater depth

Checklist

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

Affected core subsystem(s)

module

[ljharb]

would love this + a backport

[mscdex]

I'm not sure we need to use the word "revert" if the current behavior was added a very long time ago.

[bmeck]

@mscdex it has accidental side effects, I would prefer to keep that word.

[ljharb]

If it was unintentional and undocumented, it's a bug - I think the term isn't bound by recency.

[targos]

/cc @nodejs/tsc

I am in favor of this change

[BridgeAR]

I am also in favor of this. It seems like a more correct approach than the way it is currently working.

As there was no progress for a long time, I would like to have the @nodejs/tsc to have a look at this and to decide on how to move forward. There are already two TSC votes in favor.

[BridgeAR]

This needs a rebase. LGTM otherwise

[BridgeAR]

There is a new eslint rule in place that prevents messages without dynamic content (or it is at least planned). I do see that it makes sense to have a individual message here, so we should either explicitly accept the message here or maybe just change the inner.js export content to contain a property with a string, remove the message and use deepStrictEqual. With the latter the error message should be explicit enough.

[Trott]

@BridgeAR If that rule ever comes to pass, we can always disable it with a comment for the instances where a string literal makes sense. So this can stay as is, if the proposed lint rule is the only motivation for changing it.

[BridgeAR]

@bmeck I guess you actually want this to be looked at and it to get merged, right?

In that case it will need a rebase and the title should be changed accordingly.

[fhinkel]

@BridgeAR I don't see any -1. With the approvals, I think we can go ahead and merge it (after a rebase). Can we take this off the TSC agenda? Or am I missing something?

[mcollina]

LGTM with a better PR description and commit description

[BridgeAR]

@fhinkel sure, I just removed the labels. I just wanted to make sure it is properly looked at as it was originally controversial if that should be changed or not.

[BridgeAR]

Ping @bmeck

[bmeck]

I can make a better description tomorrow and I think we are approved beyond that.

[jasnell]

just double checking... this will land in v10

Should be plenty of time for that assuming it lands by end of March... :-)

[jdalton]

Anyone know about the treatment of the trailing /. In some places I see logic to handle \(for Windows). I'm wondering if it's needed here too.

Update:

Okay confirmed. Windows supports paths ending in \. and \... I think the check should be done to support both or branch per platform as is common elsewhere in Node.

[bmeck]

@jdalton

I think the check should be done to support both or branch per platform as is common elsewhere in Node.

The surrounding code is only looking at / characters outside of this PR, it looks like \\ has been swapped by the point it gets to that otherwise there would be lots of bugs in module.js.

[jdalton]

It looks like it goes

`require` then
`Module.prototype.require` then
`Module._resolveFilename` then
`Module._findPath`, with array from `Module._resolveLookupPaths`, then
// this code

The request isn't normalized through those passes yet.
The bug is likely there with the initial trailingSlash check in Node today too.

Update:

Filed the bug: #18299.

[jdalton]

👆 at the moment because the trailing slash check is for forward-slash-only, on Windows paths ending in a backslash hit that code path (which is removed in this PR). This means it will now fall through to the tryExtensions call on line 234, instead of the tryPackage call on line 243.

Note: Assumes a fixed #18299

[BridgeAR]

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

[BridgeAR]

@bmeck it seems like there is a lint error and a tests constantly fails:

not ok 1376 parallel/test-require-extensions-same-filename-as-dir
---
duration_ms: 0.171
severity: fail
stack: |-
assert.js:74
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: undefined strictEqual 'artischocko'
at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-require-extensions-same-filename-as-dir.js:31:8)
at Module._compile (module.js:668:30)
at Object.Module._extensions..js (module.js:679:10)
at Module.load (module.js:580:32)
at tryModuleLoad (module.js:520:12)
at Function.Module._load (module.js:512:3)
at Function.Module.runMain (module.js:709:10)
at startup (bootstrap_node.js:189:16)
at bootstrap_node.js:699:3

Would you be so kind and take a look? :-)

[BridgeAR]

I just rebased this, fixed the linter error and fixed the test issue. I am a bit confused that the old test is now working as before.

@bmeck can you please have a look at this?

[BridgeAR]

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

[bmeck]

@BridgeAR I'm dealing w/ family, won't get to this in next few days.

[BridgeAR]

Ping @bmeck. I hope everything is fine on your side :-)

[bmeck]

@BridgeAR I'm still pretty under water with workload due to things, nothing seems really apparent on why it would fail. I could try to revert and then git bisect if desired but won't get to it for a while.

[BridgeAR]

@bmeck I fixed the failure in 39f51aa but I wanted you to confirm that this is the right thing. To me it seems like a independent change made it possible to have this change without having a impact on any existing code.

[BridgeAR]

And I rebased and pushed the changes here. So this is good to go if you give your LG.

[bmeck]

@BridgeAR that test change looks correct, this PR should be adding trailing / effectively when going up a directory. So it should preserve upwards traversal looking at package.json with that test which uses ../.. for the require specifier. It would have had to have been ../../../module-stub for it to grab module-stub.json instead.

[BridgeAR]

Landed in 1ed36ae 🎉

[bmeck]

This PR reverts a regression from a change in behavior that deviated from the documented behavior of the module resolution algorithm. The deviation caused the module resolution algorithm to check for package.json files when traversing up to a parent directory before following the documented module resolution algorithm.

Link discussing the regression in greater depth

Checklist

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

Affected core subsystem(s)

module

[ljharb]

would love this + a backport

[mscdex]

I'm not sure we need to use the word "revert" if the current behavior was added a very long time ago.

[bmeck]

@mscdex it has accidental side effects, I would prefer to keep that word.

[ljharb]

If it was unintentional and undocumented, it's a bug - I think the term isn't bound by recency.

[targos]

/cc @nodejs/tsc

I am in favor of this change

[BridgeAR]

I am also in favor of this. It seems like a more correct approach than the way it is currently working.

As there was no progress for a long time, I would like to have the @nodejs/tsc to have a look at this and to decide on how to move forward. There are already two TSC votes in favor.

[BridgeAR]

This needs a rebase. LGTM otherwise

[BridgeAR]

There is a new eslint rule in place that prevents messages without dynamic content (or it is at least planned). I do see that it makes sense to have a individual message here, so we should either explicitly accept the message here or maybe just change the inner.js export content to contain a property with a string, remove the message and use deepStrictEqual. With the latter the error message should be explicit enough.

[Trott]

@BridgeAR If that rule ever comes to pass, we can always disable it with a comment for the instances where a string literal makes sense. So this can stay as is, if the proposed lint rule is the only motivation for changing it.

[BridgeAR]

@bmeck I guess you actually want this to be looked at and it to get merged, right?

In that case it will need a rebase and the title should be changed accordingly.

[fhinkel]

@BridgeAR I don't see any -1. With the approvals, I think we can go ahead and merge it (after a rebase). Can we take this off the TSC agenda? Or am I missing something?

[mcollina]

LGTM with a better PR description and commit description

[BridgeAR]

@fhinkel sure, I just removed the labels. I just wanted to make sure it is properly looked at as it was originally controversial if that should be changed or not.

[BridgeAR]

Ping @bmeck

[bmeck]

I can make a better description tomorrow and I think we are approved beyond that.

[jasnell]

just double checking... this will land in v10

Should be plenty of time for that assuming it lands by end of March... :-)

[jdalton]

Anyone know about the treatment of the trailing /. In some places I see logic to handle \(for Windows). I'm wondering if it's needed here too.

Update:

Okay confirmed. Windows supports paths ending in \. and \... I think the check should be done to support both or branch per platform as is common elsewhere in Node.

[bmeck]

@jdalton

I think the check should be done to support both or branch per platform as is common elsewhere in Node.

The surrounding code is only looking at / characters outside of this PR, it looks like \\ has been swapped by the point it gets to that otherwise there would be lots of bugs in module.js.

[jdalton]

It looks like it goes

`require` then
`Module.prototype.require` then
`Module._resolveFilename` then
`Module._findPath`, with array from `Module._resolveLookupPaths`, then
// this code

The request isn't normalized through those passes yet.
The bug is likely there with the initial trailingSlash check in Node today too.

Update:

Filed the bug: #18299.

[jdalton]

👆 at the moment because the trailing slash check is for forward-slash-only, on Windows paths ending in a backslash hit that code path (which is removed in this PR). This means it will now fall through to the tryExtensions call on line 234, instead of the tryPackage call on line 243.

Note: Assumes a fixed #18299

[BridgeAR]

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

[BridgeAR]

@bmeck it seems like there is a lint error and a tests constantly fails:

not ok 1376 parallel/test-require-extensions-same-filename-as-dir
---
duration_ms: 0.171
severity: fail
stack: |-
assert.js:74
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: undefined strictEqual 'artischocko'
at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-require-extensions-same-filename-as-dir.js:31:8)
at Module._compile (module.js:668:30)
at Object.Module._extensions..js (module.js:679:10)
at Module.load (module.js:580:32)
at tryModuleLoad (module.js:520:12)
at Function.Module._load (module.js:512:3)
at Function.Module.runMain (module.js:709:10)
at startup (bootstrap_node.js:189:16)
at bootstrap_node.js:699:3

Would you be so kind and take a look? :-)

[BridgeAR]

I just rebased this, fixed the linter error and fixed the test issue. I am a bit confused that the old test is now working as before.

@bmeck can you please have a look at this?

[BridgeAR]

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

[bmeck]

@BridgeAR I'm dealing w/ family, won't get to this in next few days.

[BridgeAR]

Ping @bmeck. I hope everything is fine on your side :-)

[bmeck]

@BridgeAR I'm still pretty under water with workload due to things, nothing seems really apparent on why it would fail. I could try to revert and then git bisect if desired but won't get to it for a while.

[BridgeAR]

@bmeck I fixed the failure in 39f51aa but I wanted you to confirm that this is the right thing. To me it seems like a independent change made it possible to have this change without having a impact on any existing code.

[BridgeAR]

And I rebased and pushed the changes here. So this is good to go if you give your LG.

[bmeck]

@BridgeAR that test change looks correct, this PR should be adding trailing / effectively when going up a directory. So it should preserve upwards traversal looking at package.json with that test which uses ../.. for the require specifier. It would have had to have been ../../../module-stub for it to grab module-stub.json instead.

[BridgeAR]

Landed in 1ed36ae 🎉

[bmeck]

This PR reverts a regression from a change in behavior that deviated from the documented behavior of the module resolution algorithm. The deviation caused the module resolution algorithm to check for package.json files when traversing up to a parent directory before following the documented module resolution algorithm.

Link discussing the regression in greater depth

Checklist

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

Affected core subsystem(s)

module

[ljharb]

would love this + a backport

[mscdex]

I'm not sure we need to use the word "revert" if the current behavior was added a very long time ago.

[bmeck]

@mscdex it has accidental side effects, I would prefer to keep that word.

[ljharb]

If it was unintentional and undocumented, it's a bug - I think the term isn't bound by recency.

[targos]

/cc @nodejs/tsc

I am in favor of this change

[BridgeAR]

I am also in favor of this. It seems like a more correct approach than the way it is currently working.

As there was no progress for a long time, I would like to have the @nodejs/tsc to have a look at this and to decide on how to move forward. There are already two TSC votes in favor.

[BridgeAR]

This needs a rebase. LGTM otherwise

[BridgeAR]

There is a new eslint rule in place that prevents messages without dynamic content (or it is at least planned). I do see that it makes sense to have a individual message here, so we should either explicitly accept the message here or maybe just change the inner.js export content to contain a property with a string, remove the message and use deepStrictEqual. With the latter the error message should be explicit enough.

[Trott]

@BridgeAR If that rule ever comes to pass, we can always disable it with a comment for the instances where a string literal makes sense. So this can stay as is, if the proposed lint rule is the only motivation for changing it.

[BridgeAR]

@bmeck I guess you actually want this to be looked at and it to get merged, right?

In that case it will need a rebase and the title should be changed accordingly.

[fhinkel]

@BridgeAR I don't see any -1. With the approvals, I think we can go ahead and merge it (after a rebase). Can we take this off the TSC agenda? Or am I missing something?

[mcollina]

LGTM with a better PR description and commit description

[BridgeAR]

@fhinkel sure, I just removed the labels. I just wanted to make sure it is properly looked at as it was originally controversial if that should be changed or not.

[BridgeAR]

Ping @bmeck

[bmeck]

I can make a better description tomorrow and I think we are approved beyond that.

[jasnell]

just double checking... this will land in v10

Should be plenty of time for that assuming it lands by end of March... :-)

[jdalton]

Anyone know about the treatment of the trailing /. In some places I see logic to handle \(for Windows). I'm wondering if it's needed here too.

Update:

Okay confirmed. Windows supports paths ending in \. and \... I think the check should be done to support both or branch per platform as is common elsewhere in Node.

[bmeck]

@jdalton

I think the check should be done to support both or branch per platform as is common elsewhere in Node.

The surrounding code is only looking at / characters outside of this PR, it looks like \\ has been swapped by the point it gets to that otherwise there would be lots of bugs in module.js.

[jdalton]

It looks like it goes

`require` then
`Module.prototype.require` then
`Module._resolveFilename` then
`Module._findPath`, with array from `Module._resolveLookupPaths`, then
// this code

The request isn't normalized through those passes yet.
The bug is likely there with the initial trailingSlash check in Node today too.

Update:

Filed the bug: #18299.

[jdalton]

👆 at the moment because the trailing slash check is for forward-slash-only, on Windows paths ending in a backslash hit that code path (which is removed in this PR). This means it will now fall through to the tryExtensions call on line 234, instead of the tryPackage call on line 243.

Note: Assumes a fixed #18299

[BridgeAR]

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

[BridgeAR]

@bmeck it seems like there is a lint error and a tests constantly fails:

not ok 1376 parallel/test-require-extensions-same-filename-as-dir
---
duration_ms: 0.171
severity: fail
stack: |-
assert.js:74
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: undefined strictEqual 'artischocko'
at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-require-extensions-same-filename-as-dir.js:31:8)
at Module._compile (module.js:668:30)
at Object.Module._extensions..js (module.js:679:10)
at Module.load (module.js:580:32)
at tryModuleLoad (module.js:520:12)
at Function.Module._load (module.js:512:3)
at Function.Module.runMain (module.js:709:10)
at startup (bootstrap_node.js:189:16)
at bootstrap_node.js:699:3

Would you be so kind and take a look? :-)

[BridgeAR]

I just rebased this, fixed the linter error and fixed the test issue. I am a bit confused that the old test is now working as before.

@bmeck can you please have a look at this?

[BridgeAR]

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

[bmeck]

@BridgeAR I'm dealing w/ family, won't get to this in next few days.

[BridgeAR]

Ping @bmeck. I hope everything is fine on your side :-)

[bmeck]

@BridgeAR I'm still pretty under water with workload due to things, nothing seems really apparent on why it would fail. I could try to revert and then git bisect if desired but won't get to it for a while.

[BridgeAR]

@bmeck I fixed the failure in 39f51aa but I wanted you to confirm that this is the right thing. To me it seems like a independent change made it possible to have this change without having a impact on any existing code.

[BridgeAR]

And I rebased and pushed the changes here. So this is good to go if you give your LG.

[bmeck]

@BridgeAR that test change looks correct, this PR should be adding trailing / effectively when going up a directory. So it should preserve upwards traversal looking at package.json with that test which uses ../.. for the require specifier. It would have had to have been ../../../module-stub for it to grab module-stub.json instead.

[BridgeAR]

Landed in 1ed36ae 🎉

[bmeck]

This PR reverts a regression from a change in behavior that deviated from the documented behavior of the module resolution algorithm. The deviation caused the module resolution algorithm to check for package.json files when traversing up to a parent directory before following the documented module resolution algorithm.

Link discussing the regression in greater depth

Checklist

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

Affected core subsystem(s)

module

[ljharb]

would love this + a backport

[mscdex]

I'm not sure we need to use the word "revert" if the current behavior was added a very long time ago.

[bmeck]

@mscdex it has accidental side effects, I would prefer to keep that word.

[ljharb]

If it was unintentional and undocumented, it's a bug - I think the term isn't bound by recency.

[targos]

/cc @nodejs/tsc

I am in favor of this change

[BridgeAR]

I am also in favor of this. It seems like a more correct approach than the way it is currently working.

As there was no progress for a long time, I would like to have the @nodejs/tsc to have a look at this and to decide on how to move forward. There are already two TSC votes in favor.

[BridgeAR]

This needs a rebase. LGTM otherwise

[BridgeAR]

There is a new eslint rule in place that prevents messages without dynamic content (or it is at least planned). I do see that it makes sense to have a individual message here, so we should either explicitly accept the message here or maybe just change the inner.js export content to contain a property with a string, remove the message and use deepStrictEqual. With the latter the error message should be explicit enough.

[Trott]

@BridgeAR If that rule ever comes to pass, we can always disable it with a comment for the instances where a string literal makes sense. So this can stay as is, if the proposed lint rule is the only motivation for changing it.

[BridgeAR]

@bmeck I guess you actually want this to be looked at and it to get merged, right?

In that case it will need a rebase and the title should be changed accordingly.

[fhinkel]

@BridgeAR I don't see any -1. With the approvals, I think we can go ahead and merge it (after a rebase). Can we take this off the TSC agenda? Or am I missing something?

[mcollina]

LGTM with a better PR description and commit description

[BridgeAR]

@fhinkel sure, I just removed the labels. I just wanted to make sure it is properly looked at as it was originally controversial if that should be changed or not.

[BridgeAR]

Ping @bmeck

[bmeck]

I can make a better description tomorrow and I think we are approved beyond that.

[jasnell]

just double checking... this will land in v10

Should be plenty of time for that assuming it lands by end of March... :-)

[jdalton]

Anyone know about the treatment of the trailing /. In some places I see logic to handle \(for Windows). I'm wondering if it's needed here too.

Update:

Okay confirmed. Windows supports paths ending in \. and \... I think the check should be done to support both or branch per platform as is common elsewhere in Node.

[bmeck]

@jdalton

I think the check should be done to support both or branch per platform as is common elsewhere in Node.

The surrounding code is only looking at / characters outside of this PR, it looks like \\ has been swapped by the point it gets to that otherwise there would be lots of bugs in module.js.

[jdalton]

It looks like it goes

`require` then
`Module.prototype.require` then
`Module._resolveFilename` then
`Module._findPath`, with array from `Module._resolveLookupPaths`, then
// this code

The request isn't normalized through those passes yet.
The bug is likely there with the initial trailingSlash check in Node today too.

Update:

Filed the bug: #18299.

[jdalton]

👆 at the moment because the trailing slash check is for forward-slash-only, on Windows paths ending in a backslash hit that code path (which is removed in this PR). This means it will now fall through to the tryExtensions call on line 234, instead of the tryPackage call on line 243.

Note: Assumes a fixed #18299

[BridgeAR]

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

[BridgeAR]

@bmeck it seems like there is a lint error and a tests constantly fails:

not ok 1376 parallel/test-require-extensions-same-filename-as-dir
---
duration_ms: 0.171
severity: fail
stack: |-
assert.js:74
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: undefined strictEqual 'artischocko'
at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-require-extensions-same-filename-as-dir.js:31:8)
at Module._compile (module.js:668:30)
at Object.Module._extensions..js (module.js:679:10)
at Module.load (module.js:580:32)
at tryModuleLoad (module.js:520:12)
at Function.Module._load (module.js:512:3)
at Function.Module.runMain (module.js:709:10)
at startup (bootstrap_node.js:189:16)
at bootstrap_node.js:699:3

Would you be so kind and take a look? :-)

[BridgeAR]

I just rebased this, fixed the linter error and fixed the test issue. I am a bit confused that the old test is now working as before.

@bmeck can you please have a look at this?

[BridgeAR]

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

[bmeck]

@BridgeAR I'm dealing w/ family, won't get to this in next few days.

[BridgeAR]

Ping @bmeck. I hope everything is fine on your side :-)

[bmeck]

@BridgeAR I'm still pretty under water with workload due to things, nothing seems really apparent on why it would fail. I could try to revert and then git bisect if desired but won't get to it for a while.

[BridgeAR]

@bmeck I fixed the failure in 39f51aa but I wanted you to confirm that this is the right thing. To me it seems like a independent change made it possible to have this change without having a impact on any existing code.

[BridgeAR]

And I rebased and pushed the changes here. So this is good to go if you give your LG.

[bmeck]

@BridgeAR that test change looks correct, this PR should be adding trailing / effectively when going up a directory. So it should preserve upwards traversal looking at package.json with that test which uses ../.. for the require specifier. It would have had to have been ../../../module-stub for it to grab module-stub.json instead.

[BridgeAR]

Landed in 1ed36ae 🎉

[bmeck]

This PR reverts a regression from a change in behavior that deviated from the documented behavior of the module resolution algorithm. The deviation caused the module resolution algorithm to check for package.json files when traversing up to a parent directory before following the documented module resolution algorithm.

Link discussing the regression in greater depth

Checklist

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

Affected core subsystem(s)

module

[ljharb]

would love this + a backport

[mscdex]

I'm not sure we need to use the word "revert" if the current behavior was added a very long time ago.

[bmeck]

@mscdex it has accidental side effects, I would prefer to keep that word.

[ljharb]

If it was unintentional and undocumented, it's a bug - I think the term isn't bound by recency.

[targos]

/cc @nodejs/tsc

I am in favor of this change

[BridgeAR]

I am also in favor of this. It seems like a more correct approach than the way it is currently working.

As there was no progress for a long time, I would like to have the @nodejs/tsc to have a look at this and to decide on how to move forward. There are already two TSC votes in favor.

[BridgeAR]

This needs a rebase. LGTM otherwise

[BridgeAR]

There is a new eslint rule in place that prevents messages without dynamic content (or it is at least planned). I do see that it makes sense to have a individual message here, so we should either explicitly accept the message here or maybe just change the inner.js export content to contain a property with a string, remove the message and use deepStrictEqual. With the latter the error message should be explicit enough.

[Trott]

@BridgeAR If that rule ever comes to pass, we can always disable it with a comment for the instances where a string literal makes sense. So this can stay as is, if the proposed lint rule is the only motivation for changing it.

[BridgeAR]

@bmeck I guess you actually want this to be looked at and it to get merged, right?

In that case it will need a rebase and the title should be changed accordingly.

[fhinkel]

@BridgeAR I don't see any -1. With the approvals, I think we can go ahead and merge it (after a rebase). Can we take this off the TSC agenda? Or am I missing something?

[mcollina]

LGTM with a better PR description and commit description

[BridgeAR]

@fhinkel sure, I just removed the labels. I just wanted to make sure it is properly looked at as it was originally controversial if that should be changed or not.

[BridgeAR]

Ping @bmeck

[bmeck]

I can make a better description tomorrow and I think we are approved beyond that.

[jasnell]

just double checking... this will land in v10

Should be plenty of time for that assuming it lands by end of March... :-)

[jdalton]

Anyone know about the treatment of the trailing /. In some places I see logic to handle \(for Windows). I'm wondering if it's needed here too.

Update:

Okay confirmed. Windows supports paths ending in \. and \... I think the check should be done to support both or branch per platform as is common elsewhere in Node.

[bmeck]

@jdalton

I think the check should be done to support both or branch per platform as is common elsewhere in Node.

The surrounding code is only looking at / characters outside of this PR, it looks like \\ has been swapped by the point it gets to that otherwise there would be lots of bugs in module.js.

[jdalton]

It looks like it goes

`require` then
`Module.prototype.require` then
`Module._resolveFilename` then
`Module._findPath`, with array from `Module._resolveLookupPaths`, then
// this code

The request isn't normalized through those passes yet.
The bug is likely there with the initial trailingSlash check in Node today too.

Update:

Filed the bug: #18299.

[jdalton]

👆 at the moment because the trailing slash check is for forward-slash-only, on Windows paths ending in a backslash hit that code path (which is removed in this PR). This means it will now fall through to the tryExtensions call on line 234, instead of the tryPackage call on line 243.

Note: Assumes a fixed #18299

[BridgeAR]

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

[BridgeAR]

@bmeck it seems like there is a lint error and a tests constantly fails:

not ok 1376 parallel/test-require-extensions-same-filename-as-dir
---
duration_ms: 0.171
severity: fail
stack: |-
assert.js:74
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: undefined strictEqual 'artischocko'
at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-require-extensions-same-filename-as-dir.js:31:8)
at Module._compile (module.js:668:30)
at Object.Module._extensions..js (module.js:679:10)
at Module.load (module.js:580:32)
at tryModuleLoad (module.js:520:12)
at Function.Module._load (module.js:512:3)
at Function.Module.runMain (module.js:709:10)
at startup (bootstrap_node.js:189:16)
at bootstrap_node.js:699:3

Would you be so kind and take a look? :-)

[BridgeAR]

I just rebased this, fixed the linter error and fixed the test issue. I am a bit confused that the old test is now working as before.

@bmeck can you please have a look at this?

[BridgeAR]

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

[bmeck]

@BridgeAR I'm dealing w/ family, won't get to this in next few days.

[BridgeAR]

Ping @bmeck. I hope everything is fine on your side :-)

[bmeck]

@BridgeAR I'm still pretty under water with workload due to things, nothing seems really apparent on why it would fail. I could try to revert and then git bisect if desired but won't get to it for a while.

[BridgeAR]

@bmeck I fixed the failure in 39f51aa but I wanted you to confirm that this is the right thing. To me it seems like a independent change made it possible to have this change without having a impact on any existing code.

[BridgeAR]

And I rebased and pushed the changes here. So this is good to go if you give your LG.

[bmeck]

@BridgeAR that test change looks correct, this PR should be adding trailing / effectively when going up a directory. So it should preserve upwards traversal looking at package.json with that test which uses ../.. for the require specifier. It would have had to have been ../../../module-stub for it to grab module-stub.json instead.

[BridgeAR]

Landed in 1ed36ae 🎉