Link node_modules during platform creation if it exists

[raphinesse]

After removing bundled node_modules from platforms in #6, I noticed that the platforms unconditionally copy their (now potentially missing) bundled node_modules in their create scripts. Since they all use shelljs for that, it currently does not break the creation if node_modules but only causes an error message to be displayed.

Example from cordova-android:
https://github.com/apache/cordova-android/blob/8dfddef6f9f5d00fba591a2fbe8837ccebea49a7/bin/lib/create.js#L171

A recent conversation with @erisu made me realize that we actually still need to either copy or preferably link <PROJECT>/node_modules/cordova-<PLATFORM>/node_modules to <PROJECT>/platforms/<PLATFORM>/node_modules if the former exists. Otherwise Node's module resolution would not pick up the correct dependency versions for the platform in case of version conflicts.

Task List

• cordova-android
• cordova-browser
• cordova-ios
• cordova-osx
• cordova-windows
• cordova-test-platform

Note

In the long run, we should probably try to have only native projects in platforms and keep the platforms' build scripts in the project's node_modules and run them from there. That should make things easier and cleaner IMHO.

[erisu]

@raphinesse

1. Is it important/necessary to keep the platform binaries?

apple_ios_version.bat
apple_osx_version
apple_osx_version.bat
apple_xcode_version
apple_xcode_version.bat
autotest
check_reqs
check_reqs.bat
cordova_plist_to_config_xml
create
create.bat
lib
templates
test
tests
testx
uncrustify.cfg
uncrustify.sh
update
update.bat

2. Can we remove them? Currently in question is create, but the rest can probably go as well.

I believe project creations is typically performed with Cordova CLI. I am not sure how many people actually create a project from the platform package.

Purpose of the question

The originating problems background

Once bundled dependencies were removed, when a platform is added, all of the dependencies are installed into the projects root directory, flattened. Therefore the copy command fails because the node_modules/cordova-ios/node_modules directory no longer exists.

Removing the bug

Yes, the best solution is to remove the copy command but there are still test failures to resolve.

The test failures

The failures are coming from create.spec.js, an e2e test file. The test uses the create binary to create a project. When creating a project, a path must be provided where the project will be created. In that folder, since the platform is being copied over and not installed via NPM, it needs the node_modules of the checked out platform to copy over. If it's not copied, it will fail later due to missing packages.

If I attempt to copy the node_modules directory into the temp project folder before it is called, create process will fail because it will appear as if the project already exists.

https://travis-ci.org/erisu/cordova-ios/jobs/433949167#L3190

The question

Basically, there were two options that I can think of. If CLI cordova platform add ios is the typical pattern, should the ability to create a project from the platform be valid? If not it can be removed.

If we want to maintain this option, we can update the binary to pass in a flag within the options argument. The flag could be called isNotCli: true, and then update the copy node_modules line to come from the __dirname and only called when isNotCli: true. The reason for a negated option is so we do not need to update CLI code.

Alternative flags could be: isPlatformBin: true, ...

I am thinking the binaries can be removed and the tests using the binaries as well can be removed.

If you have any confusion let me know and I will try to clear it up.

[raphinesse]

@erisu Usually I'm all in favor of reducing the surface of Cordova's public API. However, there are people that actually use platforms on their own (see one of the old open PRs in cordova-lib) and while I myself never have users it before, this actually seems like a good feature to me.

In any case, I think it would be best to not remove any more stuff in next major.

That's all just a first impression. I really need to take a closer look to make an informed decision. I hope I'll be able to do that this weekend.

And please note the updates to the issue description I recently made.

[dpogue]

An interesting problem (that we can defer for the moment) is that our dist packages won't have bundled node_modules anymore, so they won't run out-of-the-box.

Our release process is to generate tarballs and vote on them, and then publish those to npm, so we don't really want the node_modules folder to be in those packages at publish time, but we do want node_modules in the tarball that we make available for download from the website.

[janpio]

Is the tarball really a fully a supported distribution method (besides npm)? Does anybody use that? Why?

[dpogue]

To work a project that consists of a single platform, using the ./bin/create script from the tarball instead of cordova-cli.

[janpio]

OK.

[raphinesse]

From the description of this issue:

we actually still need to either copy or preferably link <PROJECT>/node_modules/cordova-<PLATFORM>/node_modules to <PROJECT>/platforms/<PLATFORM>/node_modules if the former exists. Otherwise Node's module resolution would not pick up the correct dependency versions for the platform in case of version conflicts.

The recent PRs by @erisu seem to ignore this fact. This is mandatory to not break module resolution. Or did you consider this and I am mistaken here? IMO, we need something like this in the create scripts:

// ROOT is the platform module root
const platformDependencies = path.join(ROOT, 'node_modules');

if (fs.existsSync(platformDependencies)) {
  const localPlatformDependencies = path.join(project_path, 'cordova/node_modules');
  fs.ensureSymlinkSync(platformDependencies, localPlatformDependencies, 'junction');
}

That should fix things until we find time to do it the right way (just my opinion):

In the long run, we should probably try to have only native projects in platforms and keep the platforms' build scripts in the project's node_modules and run them from there. That should make things easier and cleaner IMHO.

[dpogue]

There are a bunch of similar-sounding problems here, and I think you've identified one and @erisu handled one, but they're not quite the same problem and it probably makes sense to try to identify all the problems:

1. Existing released platforms need their node_modules copied when installing with the CLI
   This should already happen, because the copy command is unconditionally in their project create scripts.

2. Existing released platforms need their node_modules copied when installing using bin/create from a tarball
   This is currently broken because the tarballs don't include node_modules, but I think it's out of our current scope to try to fix this for existing releases.

3. Current platforms do not need their node_modules copied because they should match the CLI
   But it's not safe to rely on this, which is what I believe @raphinesse is pointing out. See case 5 below.

4. Current platforms need their node_modules copied when installing using bin/create from a tarball
   @erisu fixed part of this by ensuring that the bin/create scripts will include the command to copy node_modules. One problem that still exists is that the tarball releases on dist don't actually include node_modules, but that's probably also out of scope for this particular discussion (it should, however, get raised for discussion in terms of our release process).

5. Current platforms with a future CLI (or vice versa) need their node_modules copied when installing with the CLI
   This is @raphinesse's point, that is the complication in case 3. If we install an older platform, its dependencies might not match the versions used by the CLI, so we would need to copy the nested node_modules folder to ensure we don't break module resolution.

The PRs from @erisu solve part of case 4, but we probably need to always copy node_modules if they exist to handle case 5.

Are there any additional edge cases that I've missed?

---

Doing this "the right way", and running the platform API scripts from the node_modules folder itself is definitely something we should eventually try to move towards, but we would still need to account for case 4 that requires copying everything.

[janpio]

Currently installing a platform from a local folder (e.g. cordova platform add "C:\Projects\cordova5\cordova-android" --verbose) is broken because of this subject:
apache/cordova-cli#362 (comment)

[erisu]

@raphinesse @dpogue Can this ticket be closed or are there still edge cases missing?

[raphinesse]

I don't really have a good grasp of the current state of things. But if all platforms behave like Android (if node_modules exists then copy it to platforms folder), I believe we are fine for the upcoming major release.

---

Adding a platform straight from a tarball is probably still broken for any platforms without bundled dependencies. However, I believe we should not support that workflow anymore. But that should be discussed in another issue targeting a future major release.

[erisu]

@raphinesse If I am correct, you're referring to the standalone non-CLI usage workflow?

If I am correct, I believe we mentioned somewhere that an extra step to the current workflow is now required.

For users who use the tarball or GH repo for the standalone approach (non-CLI) must run npm i --only=prod before running the bin/create. Then the create script would copy the node_modules folder into the platform folder as it has done in the past.

Side note: It is highly recommended to use the NPM packaged tarball from the Apache Distribution. It is not recommended to use the GH repo directly or the package tarball from GH release page as these may contain the package-lock.json and additional non-essential files. The Apache Distribution contains the official release package and accurate for production usage. It will never contain the package-lock.json and should not contain non-essential files.

• Apache Cordova Platform Release Distribution