module: fix check for package.json at volume root #34595

DerekNonGeneric · 2020-08-01T20:53:18Z

This commit converts the "read package scope" algorithm's while loop
into a do-while loop.

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

lib/internal/modules/cjs/loader.js

guybedford

Thanks for looking into this one. I must admit I did try it but couldn't replicate the issue locally myself. Are your manual tests definitely working on both Windows and linux here?

lib/internal/modules/cjs/loader.js

DerekNonGeneric · 2020-08-03T02:10:51Z

I must admit I did try it but couldn't replicate the issue locally myself.

Perhaps try the following reduction? (Make sure the files actually get created.)

$ sudo echo '{ "type": "module" }' > /package.json
$ sudo echo 'export {}' > /index.js
$ node /index.js

Are your manual tests definitely working on both Windows and linux here?

I have had successful manual testing on Debian WSL.

Built this on Windows locally and it fails.

I imagine automated testing is going to involve https://hub.docker.com/_/microsoft-windows-nanoserver. 😄

GeoffreyBooth · 2020-08-03T23:30:23Z

I was able to reproduce it via #33438 (comment). To truly fix this I think you need to do that again, and then check out the CoffeeScript repo and run its tests (/path/to/this-prs/node ./bin/cake test from the CoffeeScript repo root). That would reproduce the CITGM failure. Get the CoffeeScript tests to pass with the node from your PR and you've fixed it.

guybedford · 2020-08-05T21:52:22Z

@DerekNonGeneric the above sounds like a good success criteria here to me... if you're able to do that do let us know.

DerekNonGeneric · 2020-08-12T01:10:16Z

@guybedford, sorry for the delay. Admittedly I'm having trouble interpreting the last couple of comments. Is the intention to forgo Windows support in this PR and simply ensure that POSIX filesystem root is functioning as expected? Please keep me posted! In the meantime, I'll be looking into how close this PR is to minimally addressing the referenced issue.

guybedford · 2020-08-12T01:16:58Z

I think it's just about testing that it works in Windows and posix.

DerekNonGeneric · 2020-08-12T20:42:21Z

Just as a brief update to this: the reason why it wasn't working for me on Windows was due to what seems like my two test files from the reduction having the wrong encoding. After re-creating these files w/ proper UTF-8 encoding, I was able to perform successful manual testing on Windows, so this solution does indeed work as expected.

I'm still thinking about what the best approach would be to get some automated testing done here.

DerekNonGeneric · 2020-08-12T23:18:59Z

I'm still thinking about what the best approach would be to get some automated testing done here.

I'm not exactly sure what the most appropriate strategy would be to take here. A few thoughts…

Use Docker to create a Windows VM w/ these two reduction files in the root dir
Expose the readPackageScope function for testing-only purposes and feed it a few Windows paths (or similar faking)

@jkrems, I know you've handled a lot of the ESM testing in the past, do you have a preference?

jkrems · 2020-08-12T23:36:01Z

Quite honestly: To me this would be a matter of "weigh risk of regression and impact of regression against the effort of writing and maintaining a test". The first two seem really low here which to me means: There's likely something more valuable to do with your time than to try too hard to find a great automated test.

There may be some things you can do with chroot but that would require special child process execution and wouldn't be cross-platform. So my position here would be: Document the commands you used to test it manually and call it a day.

DerekNonGeneric · 2020-08-13T00:17:46Z

The first two seem really low here […]

Agree completely.

Document the commands you used to test it manually and call it a day.

I ran the CoffeeScript tests using this PR's built executable (on Windows to boot) as @GeoffreyBooth suggested in #34595 (comment). The result follows.

passed 1456 tests in 16.26 seconds

I think this PR is ready for review now as all other checks have passed. :)

/cc @nodejs/modules-active-members

lib/internal/modules/cjs/loader.js

doc/api/esm.md

lib/internal/modules/cjs/loader.js

guybedford · 2020-08-13T02:40:00Z

@DerekNonGeneric if the comments are too much I'm happy to push some changes if you prefer.

DerekNonGeneric · 2020-08-13T02:52:59Z

@guybedford, sure, whatever would be easiest. I'd like it if we could finish this one off tonight if possible.

lib/internal/modules/cjs/loader.js

guybedford · 2020-08-13T03:39:12Z

@DerekNonGeneric sure I've pushed up a new commit with the suggestions, including @ljharb's changes again too. Please test it out if you can on the original cases as I only tested on unix here. And if you need Windows testing help let me know.

nodejs-github-bot · 2020-08-13T20:20:48Z

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

nodejs-github-bot · 2020-08-13T20:29:50Z

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

nodejs-github-bot · 2020-08-13T22:27:22Z

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

github-actions · 2020-08-15T02:11:20Z

Commit Queue failed

- Loading data for nodejs/node/pull/34595
✔  Done loading data for nodejs/node/pull/34595
----------------------------------- PR info ------------------------------------
Title      module: fix check for package.json at volume root (#34595)
Author     Derek Lewis  (@DerekNonGeneric)
Branch     DerekNonGeneric:fix/root-package-scope -> nodejs:master
Labels     ES Modules, author ready, module
Commits    1
 - module: fix check for package.json at volume root
Committers 1
 - Derek Lewis 
PR-URL: https://github.com/nodejs/node/pull/34595
Fixes: https://github.com/nodejs/node/issues/33438
Reviewed-By: Jan Krems 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34595
Fixes: https://github.com/nodejs/node/issues/33438
Reviewed-By: Jan Krems 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - module: fix check for package.json at volume root
   ℹ  Last Full PR CI on 2020-08-13T22:27:22Z: https://ci.nodejs.org/job/node-test-pull-request/32770/
- Querying data of job/node-test-pull-request/32770/
✔  Build data downloaded
   ℹ  This PR was created on Sat, 01 Aug 2020 20:53:18 GMT
   ✔  Approvals: 1
   ✔  - Jan Krems (@jkrems): https://github.com/nodejs/node/pull/34595#pullrequestreview-466996890
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

github-actions · 2020-08-15T02:22:18Z

Landed in 0347574

This patch converts the "read package scope" algorithm's while loop into a do-while loop enabling items at the filesystem root dir to be considered within the scope of a sibling package.json also at the filesystem root dir. Fixes: #33438 Co-authored-by: Guy Bedford <[email protected]> PR-URL: #34595 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Mary Marchini <[email protected]>

This patch converts the "read package scope" algorithm's while loop into a do-while loop enabling items at the filesystem root dir to be considered within the scope of a sibling package.json also at the filesystem root dir. Fixes: nodejs#33438 Co-authored-by: Guy Bedford <[email protected]> PR-URL: nodejs#34595 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Mary Marchini <[email protected]>

This patch converts the "read package scope" algorithm's while loop into a do-while loop enabling items at the filesystem root dir to be considered within the scope of a sibling package.json also at the filesystem root dir. Fixes: #33438 Co-authored-by: Guy Bedford <[email protected]> PR-URL: #34595 Backport-PR-URL: #35385 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Mary Marchini <[email protected]>

GeoffreyBooth reviewed Aug 2, 2020

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

guybedford reviewed Aug 3, 2020

View reviewed changes

lib/internal/modules/cjs/loader.js Show resolved Hide resolved

DerekNonGeneric added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Aug 12, 2020

DerekNonGeneric marked this pull request as ready for review August 13, 2020 00:17

DerekNonGeneric requested review from guybedford and GeoffreyBooth August 13, 2020 00:23

guybedford reviewed Aug 13, 2020

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

doc/api/esm.md Outdated Show resolved Hide resolved

doc/api/esm.md Outdated Show resolved Hide resolved

DerekNonGeneric commented Aug 13, 2020

View reviewed changes

doc/api/esm.md Outdated Show resolved Hide resolved

DerekNonGeneric commented Aug 13, 2020

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

guybedford reviewed Aug 13, 2020

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

ljharb reviewed Aug 13, 2020

View reviewed changes

DerekNonGeneric added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2020

This was referenced Aug 14, 2020

CI Relability 2020-08-14 JaneaSystems/nodejs-reliability#68

Open

CI Reliability 2020-08-14 nodejs/reliability#43

Open

CI Reliability 2020-08-15 nodejs/reliability#44

Open

mmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 15, 2020

mmarchini approved these changes Aug 15, 2020

View reviewed changes

mmarchini added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 15, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2020

github-actions bot closed this Aug 15, 2020

This was referenced Aug 16, 2020

CI Reliability 2020-08-16 nodejs/reliability#45

Open

CI Reliability 2020-08-17 nodejs/reliability#46

Open

This was referenced Aug 18, 2020

CI Reliability 2020-08-18 nodejs/reliability#47

Open

CI Reliability 2020-08-19 nodejs/reliability#48

Open

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

addaleax added the backport-open-v12.x label Sep 28, 2020

codebytere mentioned this pull request Oct 4, 2020

v12.19.0 proposal #35401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: fix check for package.json at volume root #34595

module: fix check for package.json at volume root #34595

DerekNonGeneric commented Aug 1, 2020 •

edited

Loading

guybedford left a comment

DerekNonGeneric commented Aug 3, 2020 •

edited

Loading

GeoffreyBooth commented Aug 3, 2020

guybedford commented Aug 5, 2020

DerekNonGeneric commented Aug 12, 2020

guybedford commented Aug 12, 2020

DerekNonGeneric commented Aug 12, 2020 •

edited

Loading

DerekNonGeneric commented Aug 12, 2020 •

edited

Loading

jkrems commented Aug 12, 2020

DerekNonGeneric commented Aug 13, 2020

guybedford commented Aug 13, 2020

DerekNonGeneric commented Aug 13, 2020 •

edited

Loading

guybedford commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

github-actions bot commented Aug 15, 2020

github-actions bot commented Aug 15, 2020

module: fix check for package.json at volume root #34595

module: fix check for package.json at volume root #34595

Conversation

DerekNonGeneric commented Aug 1, 2020 • edited Loading

Checklist

guybedford left a comment

Choose a reason for hiding this comment

DerekNonGeneric commented Aug 3, 2020 • edited Loading

GeoffreyBooth commented Aug 3, 2020

guybedford commented Aug 5, 2020

DerekNonGeneric commented Aug 12, 2020

guybedford commented Aug 12, 2020

DerekNonGeneric commented Aug 12, 2020 • edited Loading

DerekNonGeneric commented Aug 12, 2020 • edited Loading

jkrems commented Aug 12, 2020

DerekNonGeneric commented Aug 13, 2020

guybedford commented Aug 13, 2020

DerekNonGeneric commented Aug 13, 2020 • edited Loading

guybedford commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

github-actions bot commented Aug 15, 2020

github-actions bot commented Aug 15, 2020

DerekNonGeneric commented Aug 1, 2020 •

edited

Loading

DerekNonGeneric commented Aug 3, 2020 •

edited

Loading

DerekNonGeneric commented Aug 12, 2020 •

edited

Loading

DerekNonGeneric commented Aug 12, 2020 •

edited

Loading

DerekNonGeneric commented Aug 13, 2020 •

edited

Loading