Skip to content

fix: Change npm bin to npm root #407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mildaniel merged 2 commits into from
Nov 28, 2022
Merged

fix: Change npm bin to npm root #407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad6b1f1
Change npm bin to npm root
mildaniel Nov 17, 2022
70c4902
Merge branch 'develop' into fix-npm-bin
mildaniel Nov 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion aws_lambda_builders/workflows/nodejs_npm_esbuild/workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import logging
import json
from pathlib import Path
from typing import List

from aws_lambda_builders.workflow import BaseWorkflow, Capability
Expand Down Expand Up @@ -215,7 +216,8 @@ def get_resolvers(self):

def _get_esbuild_subprocess(self, subprocess_npm, scratch_dir, osutils) -> SubprocessEsbuild:
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"))
Copy link
Contributor

@moelasmar moelasmar Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how can we make sure the npm root command is available in all supported npm versions?

Copy link
Contributor Author

@mildaniel mildaniel Nov 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

except FileNotFoundError:
raise EsbuildExecutionError(message="The esbuild workflow couldn't find npm installed on your system.")
executable_search_paths = [npm_bin_path]
Expand Down
13 changes: 7 additions & 6 deletions tests/unit/workflows/nodejs_npm_esbuild/test_workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from pathlib import Path
from unittest import TestCase
from unittest.mock import ANY

Expand Down Expand Up @@ -65,7 +66,7 @@ def test_workflow_sets_up_npm_actions_with_bundler_if_manifest_requests_it(self)

def test_sets_up_esbuild_search_path_from_npm_bin(self):

self.popen.out = b"project/bin"
self.popen.out = b"project/"

workflow = NodejsNpmEsbuildWorkflow(
"source",
Expand All @@ -76,15 +77,15 @@ def test_sets_up_esbuild_search_path_from_npm_bin(self):
experimental_flags=[],
)

self.osutils.popen.assert_called_with(["npm", "bin"], stdout="PIPE", stderr="PIPE", cwd="scratch_dir")
self.osutils.popen.assert_called_with(["npm", "root"], stdout="PIPE", stderr="PIPE", cwd="scratch_dir")
esbuild = workflow.actions[2]._subprocess_esbuild

self.assertIsInstance(esbuild, SubprocessEsbuild)
self.assertEqual(esbuild.executable_search_paths, ["project/bin"])
self.assertEqual(esbuild.executable_search_paths, [str(Path("project/.bin"))])
Copy link
Contributor

@moelasmar moelasmar Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mndeveci mndeveci Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mildaniel mildaniel Nov 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def test_sets_up_esbuild_search_path_with_workflow_executable_search_paths_after_npm_bin(self):

self.popen.out = b"project/bin"
self.popen.out = b"project"

workflow = NodejsNpmEsbuildWorkflow(
"source",
Expand All @@ -96,10 +97,10 @@ def test_sets_up_esbuild_search_path_with_workflow_executable_search_paths_after
experimental_flags=[],
)

self.osutils.popen.assert_called_with(["npm", "bin"], stdout="PIPE", stderr="PIPE", cwd="scratch_dir")
self.osutils.popen.assert_called_with(["npm", "root"], stdout="PIPE", stderr="PIPE", cwd="scratch_dir")
esbuild = workflow.actions[2]._subprocess_esbuild
self.assertIsInstance(esbuild, SubprocessEsbuild)
self.assertEqual(esbuild.executable_search_paths, ["project/bin", "other/bin"])
self.assertEqual(esbuild.executable_search_paths, [str(Path("project/.bin")), "other/bin"])

def test_workflow_uses_npm_ci_if_lockfile_exists(self):

Expand Down