Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

npm install and dep resolvement needs to happen in the same dir #285

Merged
merged 2 commits into from
Sep 12, 2016

Conversation

matteofigus
Copy link
Member

@matteofigus matteofigus commented Sep 11, 2016

This PR fixes two issues:

  • When working on Respect dependency versions for components #280 we introduced the possibility to npm install dependency@version instead of just the dependency on the latest version. The bug was that multiple components using the same dep where installed multiple times. In addition, resolving the dependency when running the component from registry resulted in a dependency bug because the dependency wasn't registered correctly. This should fix.
  • The second is the fix for npm install on dev watcher doesn't work when launching runner from container dir #284 - npm install succeeds but then goes to an infinite loop. This is actually not related to Respect dependency versions for components #280 and happens when doing oc dev componentsFolder. Basically, until now npm install was installing inside components folder, and dependency were resolved on the folder the oc dev was running. So, oc dev . was always working (so, when launching the dev inside the components folder) but oc dev componentsFolder wasn't. From now, with this fix, it is decided that npm dependencies are installed in a node_modules folder relative to the oc dev and that's where they are resolved. This should make the dev watcher able to run from both locations.

The easiest way to test this stuff is to use a folder of components using dependency and manually run the acceptance tests from various locations. We can also add extra unit testing but I am not too concerned as this is mostly a CLI minor feature and shouldn't have impact on a registry.

@lobut let's have a look at this together tomorrow so that we can do some manual testing together and then merge ;)

@matteofigus matteofigus mentioned this pull request Sep 11, 2016
@matteofigus
Copy link
Member Author

Reviewed with @lobut so auto merging.

@matteofigus matteofigus merged commit b2a9dc4 into master Sep 12, 2016
@matteofigus matteofigus deleted the deps-fix branch September 12, 2016 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant