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

Separate Restoring from Preparing #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Sep 28, 2016

No description provided.

@TimBarham
Copy link

I agree that this has been a horrible source of bugs, and still isn't quite right, but I'm not convinced making this change is necessarily the right thing to do, vs just making the current intended behavior work properly (I'm also not convinced it isn't the right thing to do... still thinking it through). However, I wonder about this scenario:

  • Clone a project from git - contains no platforms or plugins, but there are platforms/plugins saved to config.xml / package.json.
  • Run cordova prepare android.
  • Cordova (presumably) complains that android has not yet been added to the project. Hopefully it is updated to be smart enough to recognize that is has been saved to config.xml / package.json, and recommends running cordova restore (or cordova-install) first.
  • At which point my response would be "You know I have to run it, so why don't you just freaking run it for me?", at which point we're back to where we are now 😄.

@stevengill
Copy link
Contributor

I like the proposal and think it would be a good fit for cordova@7.

Expected behavior for npm projects is to run npm install to grab the dependencies after you clone the repo. cordova install could do the same thing for cordova projects (under the hood it would just run npm install and do the adding the platforms/plugins step). A nice benefit is we could use cordova install to do other upgrade tasks (add package.json, copy deps from config.xml to package.json, etc)

I like the idea of notifying users that they need to run cordova install if they haven't.

@stevengill
Copy link
Contributor

Created an issue for tracking purposes. https://issues.apache.org/jira/browse/CB-11981

@stevengill
Copy link
Contributor

I'm thinking we hold off on this until cordova@8. In cordova@7, during restore, we copy relevant info from config.xml into package.json for saved platforms and plugins. Having users run a cordova install might be asking too much right now.

@filmaj
Copy link

filmaj commented Jul 19, 2017

I would like to resurrect this. Getting back into cordova for several months now, I have been really confused by the restore functionality, and why it was included in prepare. The goal of the prepare command, in its original form, was twofold:

  1. copy platform-agnostic www/ contents into platform artifact directories
  2. interpolate any application (meta)data from config.xml (and/or package.json) into platform artifact directories

Adding restore functionality to this violates its intended design.

I like breaking out restoration functionality into its own command. As a bonus, the command also happens to line up with the expected workflow for npm-based projects (i.e. clone a node project down, npm install to grab all your dependencies).

We would need to heavily blog / document this new flow, but I think it's a step in the right direction and would fix a lot of the bugs in cordova-lib today. It would also keep the design of cordova-lib/cordova-cli consistent: one module per executable command.

@janpio
Copy link
Member

janpio commented Oct 6, 2017

Absolutely agree. I did some research into the current behavior today, and it is totally not obvious - but could and should be.

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

Successfully merging this pull request may close these issues.

None yet

6 participants