Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix npm@5 bug when doing npm install #1103

Closed
wants to merge 1 commit into from
Closed

Fix npm@5 bug when doing npm install #1103

wants to merge 1 commit into from

Conversation

MTRNord
Copy link
Contributor

@MTRNord MTRNord commented Jun 15, 2017

[email protected] droped support of prepublish for npm install. prepare replaces prepublish. See npm/npm#10074

[email protected] droped support of prepublish for npm install. prepare replaces prepublish. See npm/npm#10074
@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@dbkr
Copy link
Member

dbkr commented Jun 16, 2017

The only problem with this is that it will drop support for npm < 4, ie. node < 7.3.0

@turt2live
Copy link
Member

It's worth noting that node 6 users can upgrade to npm 5 without issue.

@MTRNord
Copy link
Contributor Author

MTRNord commented Jun 17, 2017

Well maybe using both could help. Because for me is riot unusable in this case. Downgrade of npm is no option as I got another project where I use some newer features :/

@lukebarnard1
Copy link
Contributor

Surely we just point "prepare" at "prepublish" and we're done? (as in literally "prepare": "npm run prepublish")

@turt2live
Copy link
Member

turt2live commented Jul 3, 2017

That works until you have npm versions that haven't done the hard switch and try and do both:

  • Prepublish
  • Prepare
  • Prepublish
  • Prepare
  • Prepublish
  • ...

(Edit: I actually tried this, it is infinite)

@MTRNord
Copy link
Contributor Author

MTRNord commented Jul 3, 2017

Well you could just add the same as in prepublish to prepare. It would run them 2 times but not loop

@lukebarnard1
Copy link
Contributor

The alternative for now is to use nvm for different npms with different projects.

@MTRNord
Copy link
Contributor Author

MTRNord commented Jul 3, 2017

@lukebarnard1 that works only on Linux. On windows you only have one version :/

@richvdh
Copy link
Member

richvdh commented Jul 11, 2017

I'm clearly missing some context here, because when I do npm install with npm 5.0.3, it seems to run the prepublish script the same as it ever did. Indeed, things like npm/npm#17336 make me wonder if this change ever actually landed.

What am I missing?

@MTRNord
Copy link
Contributor Author

MTRNord commented Jul 12, 2017

@richvdh atleast on my windows maschine it did not run prepublish when it installs something as dependency.

@richvdh
Copy link
Member

richvdh commented Jul 12, 2017

What version of npm?

@richvdh
Copy link
Member

richvdh commented Jul 12, 2017

when it installs something as dependency.

Wait, that's expected. It's why the riot-web readme tells you to manually run npm i on the js-sdk and react-sdk.

Afaik npm does not run prepare in this situation either?

@MTRNord
Copy link
Contributor Author

MTRNord commented Jul 12, 2017

@richvdh well you not always need a full dev env. In that case you want it to build react-sdk or js-sdk anyway.

@richvdh
Copy link
Member

richvdh commented Jul 12, 2017

@MTRNord I don't understand how this patch helps.

@MTRNord
Copy link
Contributor Author

MTRNord commented Jul 12, 2017

@richvdh for eexample if you quickly want to build the master of riot and don't want to pull every repo it is usually enough to just do npm install in riot-web. But doing so it has to run the prepublish script to build the needed files. with npm@5 and node8 as combination on windows that script never gets run. That causes the buid process to fail. You will eed to go in the node_modules folder and run npm run build in there to to get the needed compiled/transpiled js files. But yes this patch only works for npm>4 :/

@richvdh
Copy link
Member

richvdh commented Jul 12, 2017

As I said in #riot-dev, you don't need to build the dependencies when setting up the master branch of riot-web.

@MTRNord said he would get back to me with details of what problem this actually solves.

@richvdh
Copy link
Member

richvdh commented Jul 19, 2017

@MTRNord I'm going to close these until we understand what's going on here.

@richvdh richvdh closed this Jul 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants