Skip npm step if node_modules already exists

[sbc100]

See WebAssembly/waterfall#670

[kripken]

Is there a downside to running npm ci? If this is just meant to save a few seconds, it might be safer to keep doing it to avoid corner cases (such as the node_modules dir being out of date, or maybe we ship only part of the files some day).

[sbc100]

You are perhaps correct... I was kind thinking that if we ever do end up a situation where the node_modules we ship is not correct in some way we really would like to know about it ASAP. But I guess you are right that this might be safer..

[kripken]

It would be good to test that the node_modules we ship is correct, I agree. How about a test that manually copies the downloaded node_modules over after the emsdk runs npm ci (so we force it to use the downloaded version) and then we verify that it works with that?

[sbc100]

I've verified locally that all the emscripten tests pass with the downloaded version of node_modules.

Also, the tests we have in scripts/test.py are now all run with the downloaded version (since this changes makes that the only option). What more testing are you envisaging other than that?

[sbc100]

For me as a developer the ability to quickly install different SDKs without running npm is something I am looking forward to.

[kripken]

What more testing are you envisaging other than that?

How about adding a test that uses closure? That would give more coverage here of npm stuff. lgtm with that.

[sbc100]

What more testing are you envisaging other than that?

How about adding a test that uses closure? That would give more coverage here of npm stuff. lgtm with that.

OK.. maybe.. but I'm not sure this repo is where we test "does the SDK work". In general "does it SDK work" is tests by the emscripten-releases builder. If anything this change makes the installed SDK more like the SDK what we already tested there, so it should require less testing than it did before this change.

another way of putting it: The requirement for a test that closure works should have been there before this change since emskd itself was installing closure. Now that emsdk just relies on the bundled version don't we need less testing :)

But I guess either way its a good smoketest.. even it it should have been there before.

[sbc100]

Added closure test