search

rokucommunity / ropm

A package manager for the Roku platform.
MIT License
31 stars 5 forks source link

feat!: Installing direct dev dependencies #45

Closed pawelhertman closed 2 years ago

pawelhertman commented 3 years ago

This PR makes ROPM logic the same as NPM which installs in the main node_modules directory:

Until now all dev dependencies weren't copied because of using the --prod flag. Both NPM v6 and v7 do return dependencies listed above by default (the only difference is that since v7 depth is set to 1 by default, but I'm fixing it in another PR).

Why direct dev dependencies should be copied by ROPM too? For example to be able to set up a unit testing framework as a dev dependency. Because of Roku's technical limitations, the majority of unit testing frameworks add some files into a package to run tests. Without this change the unit testing framework has to be set as a production dependency therefore if used in a library module we use in our frontend app, it would be automatically copied into the production app code because it would be a prod dependency of direct app's prod dependency.

PS This is the last PR of the series of changes I was working on.

TwitchBronBron commented 2 years ago

@pawelhertman i had to resolve a merge conflict. Can you verify this PR still works the way you intended? Also, could you add a unit test or two that ensures this functionality will work in the future?

pawelhertman commented 2 years ago

@TwitchBronBron yes, I confirm it works fine. It wouldn't be a unit test because it's just a change of parameters of a shell command (childProcess.execSync is not even mocked in tests). I could write another integration test however it totally depends on the execution environment - if I wrote a test and keep the prod argument it would pass on npm v6. Which version of NPM does CI use? I couldn't figure it out easily. If v6 then, IMO, it's pointless to write an integration test. BTW I won't be able to respond in next 2 weeks.