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.

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

Fix npm arguments in dist-install task #15428

Closed
wants to merge 1 commit into from

Conversation

srmagura
Copy link
Contributor

Fixes #15382 (for real). Cloning the repository and running gulp dist-install failed with the error

Error: command "npm" with parameters "install,build/dist/" exited with code 1

on Node 16.15.0, npm 8.19.1, and Ubuntu 22.04.

Credit to @cuiyc2000 for finding this fix.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Considering that the existing command works just fine for me (and always has), it's not entirely clear why we need to change this?
Again, the commit message doesn't really contain any description of the underlying problem or links to resources explaining why such a change is necessary.

@yc-cui
Copy link

yc-cui commented Sep 13, 2022

@Snuffleupagus Could you please tell us the exact node.js and npm version you are using ? Also, why you remove the browserify example?

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Sep 13, 2022

Could you please tell us the exact node.js and npm version you are using?

Considering that the existing code has worked for years, without any issues reported, there must be something that's changed (e.g. in Node.js) to necessitate changes here. Hence, as already mentioned in #15428 (review), if this change is indeed correct it ought to be possible to provide resources/links explaining why it's actually necessary.

Also, why you remove the browserify example?

How is that question relevant here!? Besides, PR #15388 already outlines in some detail why that's done.

@Snuffleupagus
Copy link
Collaborator

Unfortunately it seems that the changes in this patch breaks the following use-case, since the example doesn't load correctly:

Doing that against the master branch works just fine (at least on Windows), hence this most unfortunately doesn't look like a change we can consider here!

@srmagura
Copy link
Contributor Author

srmagura commented Sep 13, 2022

As @cuiyc2000 said, what are your versions of Node and npm @Snuffleupagus?

@Snuffleupagus
Copy link
Collaborator

what are your versions of Node and npm

$ npm version
{
  npm: '8.1.4',
  node: '16.0.0',
  v8: '9.0.257.17-node.10',
  uv: '1.41.0',
  zlib: '1.2.11',
  brotli: '1.0.9',
  ares: '1.17.1',
  modules: '93',
  nghttp2: '1.42.0',
  napi: '8',
  llhttp: '6.0.0',
  openssl: '1.1.1k+quic',
  cldr: '39.0',
  icu: '69.1',
  tz: '2021a',
  unicode: '13.0',
  ngtcp2: '0.1.0-DEV',
  nghttp3: '0.1.0-DEV'
}

Furthermore, please keep in mind that Node.js 14 is still officially supported for some time longer.


The more relevant question here, as far as I'm concerned, is if you can successfully follow the steps outlined in #15428 (comment) with this patch applied?

@srmagura
Copy link
Contributor Author

Created issue #15435 with the exact steps to reproduce this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: command "npm" with parameters "install,build/dist/" exited with code 1
3 participants