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

Look for node_modules in any recursive parent directory #44

Merged
merged 17 commits into from Sep 28, 2018

Conversation

@ariofrio
Copy link
Contributor

ariofrio commented Sep 1, 2018

Resolves #43.

This needs tests at the moment. Tests added.

ariofrio added some commits Sep 1, 2018

@raphinesse raphinesse force-pushed the ariofrio:patch-1 branch from 3d53c2a to acb1c6e Sep 2, 2018

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Sep 2, 2018

It turns out that when using resolve-from, Node's Module._pathCache is causing more trouble than it's worth. So I've added an implementation using resolve. Thanks for suggesting the package!

Do you think you could write a few tests for this, @ariofrio? That would be awesome! I think a few unit tests for pathToInstalledPackage with some local fixtures should be enough.

@janpio janpio added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Sep 5, 2018

@janpio janpio moved this from 🐣 New PR / Untriaged to ⏳ Waiting for Review in Apache Cordova: Tooling Pull Requests Sep 5, 2018

Apache Cordova: Tooling Pull Requests automation moved this from ⏳ Waiting for Review to 🙅 Pending Approval Sep 5, 2018

@raphinesse
Copy link
Contributor

raphinesse left a comment

Needs test as described in my previous comment.

@ariofrio

This comment has been minimized.

Copy link
Contributor Author

ariofrio commented Sep 5, 2018

Thanks for working on this, @raphinesse! I'm pretty swamped right now; hopefully I can find some time for in a week or so to write a few tests for this.

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Sep 12, 2018

@ariofrio FYI we're in the process of preparing the next major release. If we want this change in it, we should start work on it rather sooner than later. I'm sorry to say I can give no specific time frame though.

@ariofrio

This comment has been minimized.

Copy link
Contributor Author

ariofrio commented Sep 20, 2018

Working on this now. Not sure what a unit test for pathToInstalledPackage that mocks resolve would accomplish, since most of the functionality that needs to be tested depends on the way resolve works.

I have in mind an "integration" test that creates this directory structure:

tmpdir/
  node_modules/
    dummy-local-plugin/
  nested-directory/

And then tries to run pathToInstalledPackage to find dummy-local-plugin from 'tmpdir' and then from 'tmpdir/nested-directory'.

What do you think?

Update: Committed a version of this. Verified locally that the new tests fail on master. The first test succeeds because it tests finding the package in the current directory, which already worked.

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented on spec/fetch.spec.js in b5f9425 Sep 20, 2018

Oops, that's why the tests are failing

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Sep 20, 2018

The tests look good to me on a first glance 👍

I never expected resolve to be mocked. How you laid out the tests was exactly how I imagined it.

One last thing: it would be nice to add a test that shows that globally installed modules will be taken into account too. Not sure how to best test this, but maybe you have an idea?

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Sep 20, 2018

Or it might have been the linting errors that show up in the CI logs 😅

@ariofrio

This comment has been minimized.

Copy link
Contributor Author

ariofrio commented Sep 20, 2018

Huh, is taking into account globally installed modules expected behavior?

Seems like it would be error-prone, since running the same command on a different environment without that globally installed module would fail.

Not sure if resolve provides that functionality, anyway, reading the docs.

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Sep 20, 2018

While I agree that taking global modules into account might be problematic, I would have thought resolve covers that functionality since it's part of the node module resolution algorithm.

Any way: all the more reason to assert the presence/absence of this behavior in the current implementation.

@ariofrio

This comment has been minimized.

Copy link
Contributor Author

ariofrio commented Sep 20, 2018

Good point about needing to test it either way.


Research and links about globally-installed modules

At least on my environment, requiring a module that's installed globally doesn't work.

It seems that it's not supposed to work, unless NODE_PATH is set or the package is installed to a fixed set of three GLOBAL_FOLDERS (on Ubuntu using the nodesource distribution, globally installed modules don't go into any of them, but into /usr/lib/node_modules).

A grep through the source code seems to indicate that resolve doesn't read NODE_PATH or check the GLOBAL_FOLDERS, but expects them to be passed as the paths argument.

So to achieve full fidelity, we'd probably need to pass NODE_PATH and those three directories to resolve. Even then, in common cases globally installed modules won't be accessible.

Certainly doable. Looking for use-cases to work off of, it seems RisingStack and other use NODE_PATH to avoid relative paths in require(), but they don't use it with whole packages.


Bottom line, globally installed modules probably shouldn't work on node, but it depends on whether the node environment is setup to install global packages to one of GLOBAL_FOLDERS. I don't know if we can assume that modern distributions avoid that, though this thread suggests we can.

Supporting NODE_PATH would make cordova-fetch fully compatible with npm resolution algorithm AFAIK, and the test for it seems straightforward.

@ariofrio

This comment has been minimized.

Copy link
Contributor Author

ariofrio commented Sep 20, 2018

Added NODE_PATH support and tests.


This is the code that generates those GLOBAL_FOLDERS. It's straightforward to replicate this functionality in cordova-fetch, but the way it finds the node_prefix has changed because the node binary moved, and I don't know if it could in the future.

Unfortunately, globalPaths seems to be undocumented, though it can be accessed through require('module').globalPaths in all node versions thus far, as far as I can tell. It can also be accessed by looking at the last three items of module.path.

All of these options have drawbacks, and supporting GLOBAL_FOLDERS would be "mostly for historic reasons". @raphinesse, up to you whether to support them and which option has the best trade-offs for the cordova project.


As far as testing global modules, I only see two ways of testing it:

  1. actually install a global module and uninstall it afterwards, which could be disruptive and require sudo on some systems
  2. use NODE_PATH to simulate global modules, but this is indistinguishable from testing NODE_PATH support
@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Sep 24, 2018

Great research @ariofrio! 🏅

I think supporting local lookup plus NODE_PATH lookup is just fine. No need for the hard-coded GLOBAL_FOLDERS. I will do a code review and then we need to run some tests with cordova-lib using this version.

Tests have been added as requested

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Sep 25, 2018

Alright! I just ran the automated tests of cordova-lib using this version of cordova-fetch and everything was fine.

Apache Cordova: Tooling Pull Requests automation moved this from 🙅 Pending Approval to ✅ Approved, waiting for Merge Sep 25, 2018

@raphinesse
Copy link
Contributor

raphinesse left a comment

All good IMHO. Commits should be squashed on merge.

@raphinesse raphinesse requested review from erisu and dpogue Sep 25, 2018

@raphinesse raphinesse self-assigned this Sep 25, 2018

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Sep 25, 2018

So after really putting this to work to do dependency prefetching for the cordova-lib test suite, one problem popped up. I added a regression test and a fix for it.

@erisu

erisu approved these changes Sep 28, 2018

@@ -22,7 +22,7 @@ var events = require('cordova-common').events;
var path = require('path');
var fs = require('fs-extra');
var CordovaError = require('cordova-common').CordovaError;
const { getInstalledPath } = require('get-installed-path');
const resolve = Q.denodeify(require('resolve'));

This comment has been minimized.

Copy link
@erisu

erisu Sep 28, 2018

Contributor

Not an issue but I know there was an initiative to remove the Q dependency and use Native promises. But this is no problem for now.

apache/cordova#7

This comment has been minimized.

Copy link
@erisu

erisu Sep 28, 2018

Contributor

Everything looks good IMO.

@raphinesse raphinesse merged commit f37c7ba into apache:master Sep 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Sep 28, 2018

@ariofrio

This comment has been minimized.

Copy link
Contributor Author

ariofrio commented Sep 28, 2018

Great! Thanks @raphinesse and @erisu for taking the time to work with me on this!

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Sep 28, 2018

Thanks for your contribution @ariofrio!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.