Closed ariofrio closed 6 years ago
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.
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.
@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.
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.
The tests look good to me on a first glance :+1:
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?
Or it might have been the linting errors that show up in the CI logs 😅
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.
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.
Good point about needing to test it either way.
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.
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:
Great research @ariofrio! :medal_sports:
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.
Alright! I just ran the automated tests of cordova-lib
using this version of cordova-fetch
and everything was fine.
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.
Great! Thanks @raphinesse and @erisu for taking the time to work with me on this!
Thanks for your contribution @ariofrio!
Resolves #43.
~This needs tests at the moment.~ Tests added.