Run npm install in post-merge hook

[qwerty287]

To update JS dependencies just like Composer dependencies. Only runs in dev mode.

[ildyria]

You may also want to run mix then: https://laravel.com/docs/8.x/mix#running-mix

[qwerty287]

But don't we include the compiled JS file already in the Git repo?

[ildyria]

But don't we include the compiled JS file already in the Git repo?

But then what is the point of npm install ? :) I am not sure how this is actually done in this case, but I don't think the npm install is necessary. I may be wrong.

[qwerty287]

It updates the JS dependencies in node_modules that will then be used if you call mix. Similar to what composer install is doing with PHP dependencies. If the dependencies aren't installed, you can't use mix.

[d7415]

It updates the JS dependencies in node_modules that will then be used if you call mix. Similar to what composer install is doing with PHP dependencies. If the dependencies aren't installed, you can't use mix.

In that case, I'd suggest both or neither. I'd lean towards neither, especially as we're not really using livewire yet.

[ildyria]

The more I think of it, the more unsure I am as we commit the JS and css files. As a result we do not need to compile them each times.
Otherwise this requires an additional nodeJs dependency which I am not too happy to add for users. I would rather have php being the only requirement. :)

[nagmat84]

The more I think of it, the more unsure I am as we commit the JS and css files. As a result we do not need to compile them each times.
Otherwise this requires an additional nodeJs dependency which I am not too happy to add for users. I would rather have php being the only requirement. :)

Total agreement. I don't like adding development dependencies to the main line. But it is up to you, @ildyria, you approved this PR. If you are getting second thoughts, then you should revoke the approval. I am not a big fan of this PR.

I have already asked in #1440 (comment) why we need NPM at all, because I have never needed it before.

[ildyria]

Make this "optional" in other words, do not fail if npm is not available.

[qwerty287]

@ildyria it now only runs if npm is available.

[ildyria]

LGTM

[codecov]

Codecov Report

Merging #1445 (f90c025) into master (1121784) will decrease coverage by 0.81%.
The diff coverage is n/a.