fix: Change npm bin to npm root #407
Conversation
Just to confirm, |
That's correct. npm reference |
|
|
||
| self.assertIsInstance(esbuild, SubprocessEsbuild) | ||
| self.assertEqual(esbuild.executable_search_paths, ["project/bin"]) | ||
| self.assertEqual(esbuild.executable_search_paths, [str(Path("project/.bin"))]) |
There was a problem hiding this comment.
I tested running npm root command, and actually the output is not project/.bin as npm bin command output. my npm version is 8.1.1
I think it will not make a difference in our case, but I am sharing that with you so may be I am missing any thing
There was a problem hiding this comment.
Given that there are some discrepancies, do we have integration tests that will catch any of these possible errors? Or these differences are just fine since all integration tests have passed.
There was a problem hiding this comment.
In this case it shouldn't make a difference. npm bin would return the path to the binary directory itself which we mock and then later assert. npm root returns the path to the active node_modules, which should have a .bin directory within it. In this case we have to append that directory ourselves so that tests should essentially be equivalent.
Our integration test cases should already cover this case, we just need to run them using npm 9 in our test jobs.
| try: | ||
| npm_bin_path = subprocess_npm.run(["bin"], cwd=scratch_dir) | ||
| npm_bin_path_root = subprocess_npm.run(["root"], cwd=scratch_dir) | ||
| npm_bin_path = str(Path(npm_bin_path_root, ".bin")) |
There was a problem hiding this comment.
how can we make sure the npm root command is available in all supported npm versions?
There was a problem hiding this comment.
I checked in the documentation which versions this command is supported in https://docs.npmjs.com/cli/v8/commands/npm-root. Should be all the same versions as npm bin + npm 9.
moelasmar
left a comment
moelasmar
left a comment
There was a problem hiding this comment.
should we update our Appveyor jobs to run the nodejs build test cases in different versions
I was planning on doing this as a follow-up item. |
4 participants
Description of changes:
npm 9 has removed the
npm bincommand which was used to find the local installation of esbuild.This change switches the
npm bincommand to usenpm rootwhich should be backwards compatible.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.