Skip to content
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

Jump to bottom

chore: Remove npm version pinning and use global npm for alias installation #14

Merged
merged 4 commits into from
Aug 29, 2024
Merged

chore: Remove npm version pinning and use global npm for alias installation #14

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f7fc2be
chore: pin npm@10
BilalQamar95 Aug 20, 2024
1a89344
chore: removed redundant npm install in install_requirements_npm_aliases
BilalQamar95 Aug 28, 2024
0b4923c
chore: Use global npm for alias installation in install_requirements_…
BilalQamar95 Aug 28, 2024
c123cee
chore: removed unnecessary comment
BilalQamar95 Aug 28, 2024
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
10 changes: 2 additions & 8 deletions tubular/scripts/frontend_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,16 +106,10 @@ def install_requirements_npm_aliases(self):
""" Install NPM alias requirements for app to build """
npm_aliases = self.get_npm_aliases_config()
if npm_aliases:
# Install and pin NPM to latest npm@8 version
proc = subprocess.Popen(['npm install npm@8'], cwd=self.app_name, shell=True)
return_code = proc.wait()
if return_code != 0:
self.FAIL(1, 'Could not run `npm install npm@8` for app {}.'.format(self.app_name))

aliased_installs = ' '.join(['{}@{}'.format(k, v) for k, v in npm_aliases.items()])
# Use the locally installed npm at ./node_modules/.bin/npm
# Use the locally installed npm at ./node_modules/.bin/npm to install aliases
Copy link
Member

Choose a reason for hiding this comment

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

[question] Does this need to reference ./node_modules/.bin/npm, or could this code path rely on just npm install similar to the other existing examples of npm install?

install_aliases_proc = subprocess.Popen(
    ['npm install {}'.format(aliased_installs)],
    cwd=self.app_name,
    shell=True,
)

Copy link
Author

Choose a reason for hiding this comment

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

Code path could rely on just npm install, I have updated it

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks. With this updated, I don't think we need the Use the locally installed npm at ./node_modules/.bin/npm to install aliases comment anymore now that we're doing a standard npm install.

install_aliases_proc = subprocess.Popen(
['./node_modules/.bin/npm install {}'.format(aliased_installs)],
['npm install {}'.format(aliased_installs)],
cwd=self.app_name,
shell=True
)
Expand Down
Loading