module: fix check for package.json at volume root by DerekNonGeneric · Pull Request #34595 · nodejs/node · GitHub

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

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?

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. 😄

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.

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

@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.

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

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.

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?

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.

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

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

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

@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.

Nice, this seems to be working well according to my manual testing on Windows. Also, the CoffeeScript tests report the following (it's not a performance benchmark, so time may vary).

passed 1456 tests in 20.05 seconds

However, running ./vcbuild test on Windows doesn't work as expected. After deleting the package.json at the root dir, some other failures appeared.

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

This turned out looking pretty clean, thanks for the refactoring @guybedford. Is there anything left to do on my end that might increase confidence in this PR? If I knew how to run CITGM, I might've done that too, but at least the CI looks promising (as well as my local manual reduction test & the CoffeeScript tests on Windows).

@DerekNonGeneric if you've done your manual tests on the latest build that's all we really need here to merge. Thanks.

New reviews needed @nodejs/modules-active-members

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

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 <guybedford@gmail.com>

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

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

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

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

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 <guybedford@gmail.com> PR-URL: #34595 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Mary Marchini <oss@mmarchini.me>

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 <guybedford@gmail.com> PR-URL: nodejs#34595 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Mary Marchini <oss@mmarchini.me>

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 <guybedford@gmail.com> PR-URL: #34595 Backport-PR-URL: #35385 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Mary Marchini <oss@mmarchini.me>

GeoffreyBooth reviewed Aug 2, 2020

View reviewed changes