Allow package to make npm install optional - also fix #13 and #18

[driskell]

Hello

This PR adds ability for a package to give extra composer configuration of composer-npm-optional which prevents the composer-npm-bridge from throwing an exception if NPM is not found for that particular install. In my use case the module is able to use alternative (albeit less featureful and slower) means to provide the same functionality the NPM modules would provide. So this is a useful thing for me at least.

Additionally, it increases priority of the bridge plugin because I had an issue with installers. Most installers (magento-composer-installer for example) are priority 0. This means there's a battle between what runs first. In the Magento case, on initial install the plugin loaded last meaning the Magento installer failed because node_modules was missing. On normal runs with plugins already installed it seemed to run NPM first. Increasing priority of the NPM bridge to 1 fixed this and it always ran first before the installer, which I believe would be desirable.

I also saw #13 and #18 and implemented those. I did the preferred ENV option for disabling the bridge entirely. And allowed another composer extra key to set a custom timeout if someone has many many packages to install via NPM (composer-npm-timeout).

Happy to split the PR if required.

[driskell]

Gonna look at tests shortly.

[codecov]

Codecov Report

Merging #19 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master    #19   +/-   ##
=======================================
  Coverage       100%   100%           
- Complexity       41     55   +14     
=======================================
  Files             7      7           
  Lines           123    163   +40     
=======================================
+ Hits            123    163   +40

Impacted Files	Coverage Δ	Complexity Δ
src/NpmBridge.php	100% <100%> (ø)	22 <5> (+8)	⬆️
src/NpmClient.php	100% <100%> (ø)	17 <3> (+5)	⬆️
src/NpmBridgePlugin.php	100% <100%> (ø)	7 <0> (+1)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a32e999...8dbfd9b. Read the comment docs.

[ezzatron]

Hey, nice PR 👍

I'll do a full review soon, but I have a couple of design questions:

Do the settings being introduced here apply only to the package in which they're defined? Or do the root package settings take precedence? This probably needs clarification in the README. I would say the better approach is to allow each package to have individual timeouts and optional flags. You may have already designed it this way - just looking for clarification.

Also, it might be better to use an object for both new settings under a single key like so:

{
    "extra": {
        "npm-bridge": {
            "timeout": 9000,
            "optional": true
        }
    }
}

Having a shared prefix works okay for a couple of settings, but if this project ever grows more, I think it would quickly become cumbersome.

Could you also please run make lint? It may fail due to "risky fixers", but I've pushed a commit to master that addresses this.

[driskell]

Hello! Thanks for your time.

Yes, each package can set it’s own options. My thinking being that some packages might want to break the install if they won’t work without npm, whereas mine for instance will have a fallback so I wouldn’t want to enforce using npm. I will be sure to clarify this in the README.

I will take a look at lint if I can soon. Happy to switch to a settings object too - and I should probably use constants for the keys!

[driskell]

I've now updated. I completed all the tasks I described above.

Let me know if any other adjustments needed.

Oh - I also updated the URL for the php-cs-fixer to use https so it's a little more secure - hope that's OK.

[driskell]

Force pushed a small tweak to README - I re-read it a few times and noticed it still wasn't clear. Happy to tweak it if there's any suggestion to make sure it's clear to as many people as possible.

[ezzatron]

@driskell Okay, I've merged this, and I'll roll out a minor release with the changes soon. I did quite a bit of refactoring as well, so I'd appreciate it if you could try it out for your use-case and make sure everything is working as expected before I do that.

In terms of behavioral changes, I removed "optional" functionality from the "update" operation. Updates only happen on the root package, so it doesn't affect consumers, and doesn't make much sense to have it be optional. If you're developing a package that uses composer-npm-bridge, you should have NPM installed.

This also made me realize that I want to change the "update" behavior of composer-npm-bridge going forward. I no longer think a composer update should even trigger an npm update. This should instead be handled directly via npm or yarn by the package maintainer.

I also need to remove support for PHP 7.0 soon, because of test dependency issues in adding 7.3 support. So I'll most likely release a new major version soon afterwards, removing support for 7.0, and removing the "update" operation at the same time.

[driskell]

Thank you. I will do some testing. I agree on the update operation - if it’s just root level then yes it would be expected you do have NPM available.

[driskell]

I like the refactor - I had struggled a little on getting the responsibilities in the right place and moving the timeout to an argument is definitely better - I think I was clouded by the way ProcessExecutor works where it's static - so the new way is much cleaner. Moving the NpmNotFound exception is definitely an improvement too 👍 Thanks!

I just tested with dev-master as a dependency on my module and installed it within a Magento repository and it worked a charm - the priority ordering works fine. I also hid NPM by renaming the binary and things worked as expected - successfully installation regardless.

I'm happy with the new version 😄

[ezzatron]

Thanks! If I had more free time I would have done more rounds of PR reviews so I could discuss each refactor in more depth, but I think you get the direction I took, so that's awesome 😄

A few small things worth mentioning:

I think taking the timeout as an argument will make even more sense once I remove update from NpmClient, since there will only be install left using it. Actually looking forward to pulling that stuff out.

The Composer ProcessExecutor static stuff is a bit of a shame, I agree. The key thing you were missing there (in terms of testing/mocking) is that Phony's static mocking relies on you passing in the generated class name. To be honest I'm not sure exactly how those tests were working before - maybe because getTimeout and setTimeout were not actually being called statically despite the methods being static?

There were also a bunch of tests where you were using never()->returned() when I think you probably wanted to use never()->called().

Anyway, thanks for the help, and I'll push out a new release soon! 👍

[ezzatron]

Released in 4.1.0