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

[BilalQamar95]

No description provided.

[adamstankiewicz]

Please see my comments/suggestion related to this hardcoded install of npm in the related Github issue and Slack thread.

Incompatible version of NPM (v8) currently used to install NPM aliases during the GoCD pipeline build step in tubular (source), resulting in "Unsupported engine" warnings about using Node 16; the pinning was not accounted for during Node 18 upgrade.
a. FWIW, it's a bit odd that we re-install NPM altogether here regardless.

Notably:

FWIW, it's a bit odd that we re-install NPM altogether here regardless.

Similar to the edx-internal feedback, is there any reason why this couldn't rely on an NPM version compatible with the Node version specified in MFEs' .nvmrc file? For example, the benefit of using nvm (Node Version Manager) is that by nvm automatically switches the version of NPM when changing its current Node version (e.g., with nvm use).

is there a way we could possibly use nvm here alongside the MFEs' supported Node version in the defined .nvmrc file within the repository?

If we continue to convention of simply pinning NPM here, we run the risk of continuing to run into future issues similar to what we're facing today with regard to "Unsupported engine" warnings about using Node 16 warnings during MFE builds in GoCD, if we forget (again) to update this version override (i.e., not the greatest from a MX perspective).

[adamstankiewicz]

IMHO, it's a code smell that we're installing npm here in install_requirements_npm_aliases but not anywhere else we install dependencies.

For example, install_requirements first executes npm install and then executes install_requirements_npm_aliases, which is what overrides the default npm version.

[question] Why can we install dependencies without an npm version override within install_requirements, but not in install_requirements_npm_aliases?

[adamstankiewicz]

Related, while we're in here, that parent npm install within install_requirements really should still be npm ci, not npm install.

[context] Running npm install means we're possibly including later versions of node_modules if a dependency was not pinned in package.json, running the risk of potentially releasing an MFE with node_modules that could have a bug that would have been otherwise caught during local dev/QA.

That said, we're tried to make this change in the past per this issue and related PR, but it was unfortunately since reverted as it started to throw peerDependency errors for at least one MFE.

I still feel we should make the switch to npm ci for the above stated reason, but it's likely a change that should be communicated out to MFE owners that they may need to investigate a fix for their individual MFEs to resolve any peerDependency errors as a result of the change.

[jristau1984]

This feels like scope creep to me... @adamstankiewicz can you help me understand why we should pull in work that previously was unable to land due to unknown complexity?

[adamstankiewicz]

@jristau1984 Only the npm ci bit is scope creep (OK to defer as separate initiative, but shouldn't be forgotten/lost IMHO; that's the only reason I'm mentioning npm ci specifically).

The rest of the feedback/questions is still pertinent; relying on the supported Node/NPM versions defined by the .nvmrc file within each MFE itself is desired/preferable, without having a single code path override the global npm version, potentially to a version incompatible with the installed Node version.

[BilalQamar95]

Thank you for the feedback. The goal of this PR is to pin npm to v10 to address the immediate issues caused by the transition from node v16 to v18 and to ensure smooth compatibility as we work towards node v20. The suggestions about using .nvmrc, nvm, and switching to npm ci are definitely valid points that could help with consistency and future-proofing.

However, these changes introduce broader adjustments that I would say go beyond the immediate scope of this PR. Since they could require further investigation, I think it would make sense to handle this in a separate ticket. This way, we can thoroughly explore these improvements without delaying the npm pinning fix.

[adamstankiewicz]

[question] Why can we install dependencies without an npm version override within install_requirements, but not in install_requirements_npm_aliases?

I would still love clarification/exploration on this question from above, before simply bumping the pinned version here. Do we really need to pin at all within install_requirements_npm_aliases when other install commands don't? Has this been explored yet?

My understanding is that FrontendBuilder should be relying on the Node version configured via the pipeline template that executed the frontend_build.py script. Is this not true?

Might it make sense to evaluate which NPM version is already available in this code path before simply bumping the pinned npm version to understand whether the pinning is even needed at all, given the other install commands don't pin? For example, could we log the current Node/NPM versions before pinning npm to see if we're actually already running the intended Node version here (sans pinning)?

---

Based on the Git blame for why this npm install npm is even within install_requirements_npm_aliases to begin with is due to when some builds were using NPM 6, which didn't support NPM aliases. Given we're well beyond NPM 6 at this point, there's a really good chance the pinning is just not needed anymore.

# Install the latest version of npm that supports aliases (>= 6.9.0)
proc = subprocess.Popen(['npm install npm'], cwd=self.app_name, shell=True)

---

tl;dr; removing the npm pinning altogether might have the same effect as pinning npm to latest v10 version. Seems worthwhile to explore before simply proceeding with bumping the pinned npm version. I don't feel it's really much additional scope to explore this question...

[BilalQamar95]

Thank you for pointing this out. You're right to question whether we still need to pin npm in install_requirements_npm_aliases, especially since other install commands don’t do this. The original reason for pinning npm seems to be to ensure compatibility with npm aliases, which earlier versions didn’t support. However, as you have mentioned, given that we're now well beyond npm 6, it's likely this pinning is no longer necessary. I’ll go ahead and explore this further.

[adamstankiewicz]

[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,
)

[BilalQamar95]

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

[adamstankiewicz]

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.

[adamstankiewicz]

LGTM, with 1 nit about the remaining comment.

[adamstankiewicz]

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.