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

Please restore --nofetch flag to support Yarn #303

Closed
kirillgroshkov opened this issue Aug 24, 2018 · 31 comments
Closed

Please restore --nofetch flag to support Yarn #303

kirillgroshkov opened this issue Aug 24, 2018 · 31 comments
Labels

Comments

@kirillgroshkov
Copy link

We're using Yarn and without --nofetch our node_modules folder gets screwed by npm (invoked by cordova-fetch). We would really really like --nofetch back to be able to work.

@raphinesse
Copy link
Contributor

Actually, this could have been fixed by apache/cordova-fetch#24. Could you please check if your problem persists when using a recent nightly build? cordova@8.0.1-nightly.2018.8.26.9a2be3df should work.

@kirillgroshkov
Copy link
Author

Looking at the PR and description I don't believe it work (cause e.g we don't have plugins in node_modules at all, since we don't use cordova-fetch).

Also, I don't feel comfortable to test nightlies, cause something else can be broken.

But I'm ready to test when the next release is shipped! Will it be release of cordova (cli) package?

@raphinesse
Copy link
Contributor

Looking at the PR and description I don't believe it work (cause e.g we don't have plugins in node_modules at all, since we don't use cordova-fetch).

Well, then where does yarn install your dependencies if not in node_modules?

You should probably clarify your use case. Because I don't think --no-fetch will be coming back in its previous form. But maybe your use case can still be handled. What do you want to do and what do you want to happen? Step-by-step instructions help tremendously.

Also, I don't feel comfortable to test nightlies, cause something else can be broken.

It won't set your machine on fire, I promise 😉. But seriously, I am only asking you to test it, not to use it in production.

But I'm ready to test when the next release is shipped! Will it be release of cordova (cli) package?

The change will be part of the next Cordova CLI major version. Testing it only after that has been released might be a little late. The earlier we know what you really need, the earlier we can start working on it 😉

@kirillgroshkov
Copy link
Author

Thanks for detailed comments!

Currently we use cordova@7.1 because it has --nofetch working. It fetches plugins into plugins directory (not in node_modules).

What we would like to have:

  • To be on the latest version of Cordova
  • To keep using Yarn (and yarn.lock) as we do (and not use npm / package-lock.json)

Cordova 8 doesn't have --nofetch, so when we run e.g cordova platform add android it "screws" our node_modules folder, cause Yarn and npm are incompatible in a way they store files in node_modules.

Since I see the effort to help from your side - I'll do my best to test it with the nightly build as you suggested!

@kirillgroshkov
Copy link
Author

kirillgroshkov commented Aug 31, 2018

I tried nightly, previously cleaned platforms and plugins folders, but kept node_modules and yarn.lock intact. Here's the result of running cordova platform add android:

Click to view output
Using cordova-fetch for cordova-android@7.1.0
Adding android project...
Creating Cordova project for the Android platform:
	Path: platforms/android
	Package: com.naturalcycles.cordova
	Name: Natural_Cycles
	Activity: MainActivity
	Android target: android-27
Subproject Path: CordovaLib
Subproject Path: app
Android project created with cordova-android@7.1.0
Android Studio project detected
Android Studio project detected
*** /Users/kirill/Idea/NCApp3/cordova-hooks/before_prepare.js started ***
Command failed: yarn ts-node -r tsconfig-paths/register ./src/scripts/addSourceMappingUrl.ts --dir /Users/kirill/Idea/NCApp3/www/build
module.js:549
    throw err;
    ^

Error: Cannot find module 'minimist'
    at Function.Module._resolveFilename (module.js:547:15)
    at Function.Module._load (module.js:474:25)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/kirill/Idea/NCApp3/node_modules/ts-node/dist/bin.js:9:16)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
error Command failed with exit code 1.

It broke on one of our cordova hooks, because previously it "screwed" node_modules, so package minimist disappeared (it's not even our dependency, it's some sub-dependency of something). It generated package-lock.json as a proof that it used npm underneath, and it's known that you cannot run npm when you have node_modules folder installed by Yarn.

So, problem is there.

@raphinesse
Copy link
Contributor

raphinesse commented Aug 31, 2018

@kirillgroshkov Thanks for testing! Could you please re-run your experiment using --verbose (i.e. cordova platform add android --verbose) and share the output of that and running cordova info in your project root? Always using the nightly. That would be great!

Edit: Oh, and please be sure to do a yarn install before, so yarn actually installs the cordova dependencies from your package.json.

@dpogue
Copy link
Member

dpogue commented Aug 31, 2018

To provide a bit of background here, there are several reasons that --no-fetch will not be coming back, but I believe what we want to accomplish should be compatible with Yarn.

The old installation code was unfortunately tied to npm2, which is several years old and relies on versions of dependencies with known security vulnerabilities. Because npm2 was installed as a dependency, running npm scripts within projects using Cordova would run with npm2 rather than the system npm. This was a frequent source of problems for users of Cordova, because npm2 and npm3 have different behaviour around peerDependencies.
The previous code was also doing custom behaviour that bypassed npm's caching helpers, didn't perform all the same verifications on the downloaded packages (which meant packages could get corrupt, and stick around), and was essentially us trying to poorly replicate a bunch of npm's behaviour. The code was unmaintainable and a source of continued bugs.

The way forward for Cordova is to have platforms and plugins listed in package.json and installed into node_modules. When you ask Cordova to install something, it will use npm to install it if it is not already installed.

From what I understand, this means it should be possible to make this work with Yarn, but you'll need to add the platforms to your package.json manually, and install them with Yarn.

For instance, with the following in your package.json:

{
  "dependencies": {
    "cordova-android": "^7.1.0"
  },
  "cordova": {
    "platforms": [
      "android"
    ]
  }
}

After you run yarn install, you should be able to run cordova prepare (on the nightly build) to create the platforms/android folder without npm being invoked.

@kirillgroshkov
Copy link
Author

@dpogue Great explanation! And I totally agree that instead of reinventing different package management methods it's easier for everyone to just stick to the same one that is also battle-tested (NPM), to have everything inside node_modules. Totally makes sense. The only missing part is then to make it work with Yarn (second-popular package management system). There's also 3rd most popular (PNPM) which I personally don't care about, but some people probably do.

Looks like the issue is:

  • To run Yarn instead of NPM to install missing packages (detect it e.g by having yarn.lock present, instead of package-lock.json).
  • To not reinstall plugins when they are already installed. (But that is less of a problem, cause even if you call yarn again one more time and nothing is changed - yarn will detect it itself and it'll take it no more than 1 second to return)

@leandro-manifesto
Copy link

Hello guys, here at my company we are using Cordova (and Ionic) to make some mobile apps.

We have a Jenkins agent doing Android builds with the following steps:

  • npm install
  • rm -rf platforms/ plugins/
  • cordova prepare android
  • cordova compile android --release

We have a mix of registry, git and local plugins and currently the prepare step takes a long time. From what I could understand cordova uses npm to install everything again, even though the install step already placed everything inside node_modules.

I tried using Yarn and pinning cordova-fetch to the patched version (apache/cordova-fetch@1087010) but npm is still being called for git and local dependencies.

I can't help but wonder if it wouldn't be better if cordova ditched the dependency manager role and only used the node resolution algorithm to find the referenced plugins (and platforms), allowing the user to pick whatever package manager he prefers.

@kirillgroshkov
Copy link
Author

@leandro-manifesto totally agree, cordova should do it's job and not try to do npm's (or yarn's or pnpm's) job.

@dpogue
Copy link
Member

dpogue commented Oct 12, 2018

I would love to do that, but people need to be able to run commands like cordova platform add and cordova plugin add, and that needs to add the platform/plugin to package.json and install it into node_modules. We need to use npm to do that.

What we can do (and I'm working on this) is to not run any package management commands as part of cordova prepare. We should tell people to run npm/yarn/pnpm themselves and then look for what we need in node_modules, and warn if we can't find it.

@kirillgroshkov
Copy link
Author

kirillgroshkov commented Oct 13, 2018

@dpogue

Just an idea, maybe cordova platform/plugin add should just add lines to configuration file (package.json) and then user can just use his package manager (npm/yarn/pnpm) to get dependencies installed (running npm i / yarn / etc)?

What we can do (and I'm working on this) is to not run any package management commands as part of cordova prepare. We should tell people to run npm/yarn/pnpm themselves and then look for what we need in node_modules, and warn if we can't find it.

Oh, that would be awesome, it'll solve the problem for so many people!

@paulsouche
Copy link

Hi,

Can't wait for this too ! But what about yarn workspaces and plug and play ? Dependencies won't always be local.

Thanks anyway for the great job

@marckassay
Copy link

An alternative to @dpogue recommendation, is installing and configuring spypkg so that npm will be mapped to yarn. See configuration here.

@kirillgroshkov
Copy link
Author

Crazy that such complex solutions as spypkg are developed to address this issue:) Should indicate the need for native yarn support!

If Capacitor (https://capacitor.ionicframework.com/) will reach GA sooner - we'll probably switch to it instead of waiting for yarn support here...

@janpio
Copy link
Member

janpio commented Jan 4, 2019

@kirillgroshkov Cordova is Open Source - feel free to invest the time and submit a Pull Request if you really need this feature.

@cmckni3
Copy link

cmckni3 commented Feb 25, 2019

Would the work need to be done in cordova-lib? cordova-cli appears to be a wrapper to me.

@raphinesse
Copy link
Contributor

The feature as requested here will most likely not be re-implemented. We are however aware of the fact that people want to use different package managers for various reasons. We plan to cater to these needs by doing less package management ourselves like @dpogue wrote in #303 (comment). However, this is a huge task and we are seriously short on developers that have the time and knowledge to tackle it. Almost all work on Cordova is done by volunteers in their spare time.

@kirillgroshkov
Copy link
Author

Sad, but I understand. We'll need to stay on Cordova 7.1 then and keep using --nofetch

@raphinesse
Copy link
Contributor

@kirillgroshkov Did you test Cordova 9? cordova-fetch never should call npm if all your Cordova dependencies are already installed and all of them are referenced by version (i.e. from registry).

@kirillgroshkov
Copy link
Author

I haven't tried Cordova 9 yet. How does cordova-fetch detect that "dependencies are already installed"? Right now we install to plugins folder (not to node_modules, due to reasons mentioned in this thread). If it tries to detect dependencies in node_modules and we install them with yarn - will cordova-fetch be able to detect that?

@raphinesse
Copy link
Contributor

@kirillgroshkov Cordova will follow the standard module resolution rules of Node.js (details in apache/cordova-fetch#43 and the PR linked therein), starting in your project root.

@kirillgroshkov
Copy link
Author

kirillgroshkov commented Apr 14, 2019

@kirillgroshkov Did you test Cordova 9? cordova-fetch never should call npm if all your Cordova dependencies are already installed and all of them are referenced by version (i.e. from registry).

Since when (which version of Cordova) is this logic in cordova-fetch applied? When we tried Cordova 7.1 and 8 - it did invoke npm install despite the fact that all plugins from config.xml were already installed in node_modules.

@raphinesse
Copy link
Contributor

9

@kirillgroshkov
Copy link
Author

9

Allright. Thanks for the info! I'll try 9 when I have time. If your description is correct - then it's great news and hopefully we can make it work with yarn (by preinstalling all dependencies with yarn and cordova-fetch should not interfere with package management in that case).

@raphinesse
Copy link
Contributor

@kirillgroshkov Hope it works for you. When running whatever cordova commands you use, include the --verbose switch and watch out for these log messages. One of these pops up every time cordova-fetch installs a package (as opposed to using an already installed one).

@cmdickson
Copy link

@kirillgroshkov Did you test Cordova 9? cordova-fetch never should call npm if all your Cordova dependencies are already installed and all of them are referenced by version (i.e. from registry).

So, this would exclude any local plugins? The behaviour I am seeing is that npm is always invoked for plugins in package.json that use file:...

@raphinesse
Copy link
Contributor

@cmdickson yes, we always need to invoke npm for local plugins.

@vzts
Copy link

vzts commented Sep 16, 2019

Just verified that cordova v9.0.0's cordova prepare under pre-yarn install environment do not trigger cordova-fetch. It works nicely with yarn. Thanks for the guidance @raphinesse.

@QuentinFarizon
Copy link

QuentinFarizon commented Jun 14, 2020

@vzts @raphinesse I can confirm that npm install is not called when pre-yarn installed environment

BUT npm install is called when a cordova plugin has been added using yarn add with a git repository url instead of the registry name (screwing node_modules as describes in this issue).
Ex : yarn add git+https://github.com/videmort/cordova-plugin-referrer.git
Vs standard yarn add cordova-plugin-referrer

This is a normal way to install dependencies, and useful when installing forks of cordova plugins.
The issue here makes it impossible to have yarn + plugins referrenced by git urls

Please either re-add --nofetch, or make it use yarn if configured as such, or just check by looking in node_modules/cordova-plugin-referrer instead of a more complicated way (which fails here)

@QuentinFarizon
Copy link

I have worked around this issue in a funny way : by running a hook that restores node_modules after cordova has finished messing with it.

In config.xml :

    <hook src="hooks/before_build/run_yarn.js" type="before_build" />

hooks/before_build/run_yarn.js :

#!/usr/bin/env node

var exec = require('child_process').execSync;

module.exports = function(context) {
  exec('rm package-lock.json || echo "No package-lock.json to remove"');
  exec('yarn');
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests