fix(cli): resolve node modules

ptitjes commented 5 years ago

Fixes #865. Fixes #792.

This PR is quite big but it does a few changes in a consistent manner:

It introduces an asynchronous resolveNode(config, ...pathSegments) function that resolves the path formed by the pathSegments in a node_modules folder. It tries to go up to the filesystem root until it finds an existing corresponding element in the filesystem.
It uses resolveNode everywhere fs.resolve was used to resolve a file in a node module.
It fixes the creation of the references in the generated gradle files.
It removes the hard-coded references in the android template and adds those to the generated file. (This is a BREAKING CHANGE!)
It removes the use of npm list to check for the relative versions of the cli and platform modules.

With this PR, npx cap sync works correctly and the gradle builds work correctly.

The iOS counterpart has not been worked on, as I can't test it.

ptitjes commented 5 years ago

@mlynch Would you mind to review ?

mlynch commented 5 years ago

Thanks for the PR. This one is pretty complex so I don't think it's going to get merged with much haste.

Do these changes impact iOS? i.e. if we merged this right now would it break iOS?

My biggest concern is the android gradle template, as you mentioned that's a breaking change. However, breaking changes are better done today than after we ship 1.0...

imhoffd commented 5 years ago

Should also resolve https://github.com/ionic-team/capacitor/issues/792

ptitjes commented 5 years ago

@mlynch, I was forced to do all these changes because the current code is a spaghetti of manual node module resolution.

No, I believe this does not impact iOS. In fact, it does run exactly the same way (whether Android or iOS) for anyone that does not use a monorepo. Indeed, the node module resolution implemented in this PR, even if smarter, eventually resolves to modules in the node_modules directory of the current working directory, if it exists.

Please, read the specification. The current code simply does not conform to the specification. I would say that these changes (or equivalent changes) are mandatory and should be implemented for the remaining parts (the iOS or electron specific code, etc).

As a consequence, you should indeed better change the gradle template (that contains hardcoded relative file paths!!) now, before it becomes unmanageable when you'll have lots of users. Monorepo-oriented tools are more and more common. You would maybe be forced to quickly release a 2.0 :D

I'm sorry if my comment sounds harsh, but the thing is that you yourself at Ionic use a lot of monorepos, so I believe you know the drill...

ptitjes commented 5 years ago

@dwieeb Ah, I missed it when I searched the issues... :P

ptitjes commented 5 years ago

@mlynch Oh, and btw, Angular is monorepo/workspaces-compatible since 7.0.0-rc2.

ptitjes commented 5 years ago

@mlynch Don't hesitate to ask me if/when you want that I rebase !

ptitjes commented 5 years ago

@mlynch, @jcesarmobile any chance this can make it to master for the next release ?

imhoffd commented 5 years ago

@ptitjes If you bump the engine requirement to Node 8.9.0+ you can use the paths option of require.resolve(): https://nodejs.org/dist/latest-v8.x/docs/api/modules.html#modules_require_resolve_request_options

This would be preferred.

ptitjes commented 5 years ago

@dwieeb Thanks! I did not know that API.

Tell me if I'm wrong but, from the docs, I get that only the second (optional) argument paths exists since Node 8.9.0. The function seems to exists since 0.13.

We don't need that paths argument here. As I understand it, it is used to specify the locations from where to run the resolution algorithm. Yet we only want the default resolution paths to be used (./, ~/.npm/, /usr/local/node, ...).

So I will rewrite my resolve() function based on require.resolve() and path.*(). Thanks for the suggestion.

Also I will add a commit to fix the requirement reported in the error you mention https://github.com/ionic-team/capacitor/issues/792#issuecomment-432855490.

imhoffd commented 5 years ago

Yet we only want the default resolution paths to be used

It looks like you're using the function to resolve node modules from config.app.rootDir. I think require.resolve('module', { paths: [config.app.rootDir] }) should work.

ptitjes commented 5 years ago

@dwieeb Yes that's finally what I went. However, I just tested it and that won't work: Unfortunately, require.resolve can only resolve packages that have a main property in their package.json. @capacitor/android don't have one (and that would make no sense to have one anyway).

I looked for possible dependencies that do that without luck. I have to resort to implementing the algorithm by hand. :(

ptitjes commented 5 years ago

@mlynch, @jcesarmobile, I rebased and took the opportunity to clean more my code and split into multiple commits. I think that this, and the fact @jcesarmobile moved the cordova plugin root, makes the commits a lot more understandable.

The only two remaining explicit references to node_modules are in the config.ios.capacitorRuntimePod and config.ios.capacitorCordovaRuntimePod variables. I don't really know how those should be handled.

Please make any comments that could help me enhance this PR. This PR is vital for me and, I believe, those that maintain large projects with dependency libraries and tools inside monorepos.

Side note: While passing over that code, I believe that since @jcesarmobile moved the cordova plugin root and made it configurable, the ./capacitor-cordova-android-plugins/ path in the android-template should not be hardcoded but generated in capacitor.settings.gradle at update.

EDIT I tested this by doing cap add android on both a simple app and a monorepo app.

jcesarmobile commented 5 years ago

For iOS we can remove those variables and replace them with the resoultion of @capacitor/ios path. I can do it in a separate PR if you don't have a mac to test.

The cordova plugin root is not really configurable, I just put the variable in the config file to reuse it instead of using the name to avoid errors, but can't be configured. We can change it to the capacitor.settings.gradle, but I don't think we will change the name/path again, so as you wish.

BTW, this change causes problems on windows. Paths like this don't work on gradle project(':capacitor-android').projectDir = new File('..\node_modules\@capacitor\android\capacitor') should be like this project(':capacitor-android').projectDir = new File('../node_modules/@capacitor/android/capacitor/')

ptitjes commented 5 years ago

I do have a Mac but I run it on Linux 😁 I am not very comfortable to develop on Mac however. So maybe it is better if you wish to handle it. Tell me and if necessary, I will make an effort.

Ah, yeah I see the config act as a central point to share paths, like the app root. So then it is good.

For the Gradle paths on Windows, just to make it clear, they must be Unix-like and not Windows-like. Do I understand correctly ? If yes, I will see how I can fix that then.

jcesarmobile commented 5 years ago

Yeah, gradle doesn't like windows paths, threats the \ as escape char and doesn't allow it.

ptitjes commented 5 years ago

@jcesarmobile I opted for the simplest way, by replacing occurrences of \ by /, as there is no way to force the path module to use the / separator. I think it should work like that but I can't test on Windows.

ptitjes commented 5 years ago

@jcesarmobile I fixed the changes you requested. That should be good now.

As of capacitor add android hanging on Windows, I have no idea what the problem could be in these commits. I checked on Linux and it doesn't hang whether @capacitor/android is already installed or not!

ptitjes commented 5 years ago

@jcesarmobile I took the liberty to add three more commits, in order to test the fixes:

Fix the android tests by using npm install to setup the node_modules directory (otherwise, when cap add android executes npm install --save @capacitor/android, node_modules/@capacitor gets wiped) and copying the cordova-android-plugins after cap add android (otherwise it fails).
Bump Jest to get describe.each.
Add tests for the mono-repo layout.

ptitjes commented 5 years ago

Any feedback ? Should I add the tests in a different PR ?

jcesarmobile commented 5 years ago

Sorry, didn't have time to review the test changes, I would prefer if you send them in a separate PR as this was very close to being merged and the tests changes will require a new review.

On a quick look, couldn't you use the existing runCommand function from cli instead of creating a new one? And on npm install, wouldn't it better to install using the relative path to cli and core instead of @capacitor/core and @capacitor/cli as that will install previous published version instead of using current code? Wouldn't affect much at the moment because we don't really do anything with them at the moment in any test, but might in the future.

jcesarmobile commented 5 years ago

@ptitjes so, are you planning on removing the tests and adding them in a separate PR?

ptitjes commented 5 years ago

@jcesarmobile Yes but I had no time to do it this week-end. I'll try tomorrow morning. Otherwise, if you want to merge the first commits, could you cherry-pick them ?

ptitjes commented 5 years ago

@jcesarmobile So I forced-pushed to remove the test commits. I will make a PR as soon as this PR gets merged. Sorry for the noise and time loss...

mlynch commented 5 years ago

w00t! Thanks so much for the PR

imhoffd commented 5 years ago

🎉

ptitjes commented 5 years ago

Thank you for your patience guys ! <3

ionic-team / capacitor

fix(cli): resolve node modules #867