Add npmInstall task

[steinybot]

This fixes #258.

I went with a more conservative approach which was to introduce a new task. We briefly discussed the removal of the staging of javascript resources from npmUpdate but this would have much more severe consequences for backwards compatibility.

I tested it out using https://github.com/scalacenter/scastie to make sure that the changes to scalaJSBundlerPackageJson to used dependencyClasspath didn't have any adverse affects. I first updated it to use 0.13.1 and generated the package.json and then updated to local build and there were no differences. I'm not sure if it is worth doing more thorough testing for this, it seems quite unlikely that change would have any negative impact, let me know what you think.

[sjrd]

This doesn't introduce any dependency between npmUpdate and npmInstall, which means they'll duplicate work and potentially run in parallel, likely causing races. It doesn't seem to me that this is the correct solution.

[julienrf]

Thanks @steinybot, I really appreciate the work you did on the documentation side! I second @sjrd, though: can we make the npmUpdate task reuse npmInstall? Also, I think we should maybe find better names… The distinction between what npmUpdate and npmInstall do is not clear just looking at their names.

[steinybot]

Ok I've made a few changes. I'm still not too sure if the names are that great. I opted to deprecate the old npmUpdate as it was the most misleading. Let me know if you prefer something else.

I'm not sure why the idle task detection doesn't seem to work reliably on the build. It works every time I run it. I have removed the negative test.

[steinybot]

I'm not too sure why the builds are failing. I thought it might have been the test I added but it seems not. It fails at random places, quite often when installing NPM dependencies, saying "No output has been received in the last 10m0s".

Most of the tests are reasonably fast and I can't see how any of the changes could have caused an intermittent failure. There was one test which I ran locally which did take over 10 minutes to download the npm dependencies. Take https://travis-ci.org/scalacenter/scalajs-bundler/jobs/415063608 for example, it failed getting sbt.

[steinybot]

@julienrf anything else I need to do on this?

[julienrf]

Hey, I will probably not be able to review this PR before next week.

On August 23, 2018 10:26:37 PM GMT+02:00, Jason Pickens notifications@github.com wrote:

@julienrf anything else I need to do on this?

[steinybot]

No problem, just checking it wasn't waiting on me.

[steinybot]

@julienrf Sorry to keep bugging you but any chance of getting this in?

[julienrf]

Don’t be sorry. I am. I forgot about this PR. I will review it in the next days.

[steinybot]

@julienrf so remove the deprecation warning on npmUpdate? Should I also remove npmInstall or keep it? They currently do the same thing.

[julienrf]

@steinybot Oops, sorry for the delay I forgot to answer you… Yes, I would remove npmInstall, what do you think?

[steinybot]

@julienrf I have removed the deprecation warning and npmInstall.