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

added cordova8 proposal #72

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

Conversation

stevengill
Copy link
Contributor

Some ideas we have for the next major version of cordova. Thoughts?

Let me know what points need discussing in more detail.

@dpogue
Copy link
Member

dpogue commented Aug 2, 2017

To the extent that we're using npm now for managing platforms and plugins, do we actually need a cordova install command or is npm install sufficient? I guess the issue is the step of actually creating the platforms/[whatever] folders?

@stevengill
Copy link
Contributor Author

Removing platforms is a much bigger refactor I think. All the platform create scripts need to output the templates, cordova.js, other build artifacts to some location. We do directly call the create scripts from node_modules/<platform>, but those create scripts then output to the platforms directory. Platforms directory is essentially our dist/build directory. You can get access to generated Xcode/android studio projects. It could be something we look at down the line though.

Just realized I should probably also add moving plugin/platform dependencies out of config.xml and have them only in package.json. Stop the auto syncing of config.xml and package.json. This currently happens during the restore step in cordova prepare.

@dpogue
Copy link
Member

dpogue commented Aug 3, 2017

Yeah, I'm not currently suggesting getting rid of the platforms folder, just wondering if it is created by cordova install or by cordova prepare? If it is created by cordova prepare, do we need cordova install at all?

@filmaj
Copy link

filmaj commented Aug 3, 2017

I am in favour of all of these points. But oof, that seems like a whole lotta work. All for the better, I think!

@dpogue on the topic of install vs. prepare, I'd like to point out that stuffing more of this functionality into prepare is what spurred the discussion in the first place, as per a comment on the install proposal: "I agree that [restore] has been a horrible source of bugs, and still isn't quite right."

@stevengill brought up a good point in that discussion that install feels like the right place to do project-setup type actions, rather than again stuffing into prepare.

Finally, I'd like to point to my comment in that proposal to highlight that we should strive to keep the design of cordova-lib consistent. In order keep the prepare command true to its initial design, we should keep pre-compilation steps encapsulated in prepare (needed due to the abstracted-above-platform-specifics nature of cordova). As @stevengill pointed out, providing an API like install keeps the workflow or setup-specific tasks like pulling in dependencies and creating project configuration separate from prepare's pre-compilation/build tasks.

To your question about whether we can get away with just running npm install: I think encapsulating install/post-install commands in a cordova install comamnd, and mirroring npm in that way, provides us a great stepping stone for eventually automatically running cordova install after an npm install (via npm scripts, for example). Worth exploring that more.

I would suggest any further discussion on the implementation details / specifics of install be debated in #54 instead of here. We can always bring the decisions / suggestions from that discussion back here (where prepare vs. install is only a subset of the larger changes being deliberated).

* Reduce maintenance burden of cordova tooling
* Refactor cordova-lib
* Improve unit test coverage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on the proposal! I really like the idea of adding more unit tests! 👍


* Remove deprecated platforms (non platform api compatibile platforms).
This is just a continuation of what we started in `cordova@7`
* **Do Now** deprecate `webos` (in cordova@7)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


* Remove `cordova platform save` command
This command isn't necessary anymore since we save by default since `cordova@7`. It was previously used to save added plugins and platforms to `config.xml`.
* **Do Now** add deprecation notice for next `cordova@7` release.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* **Do Now** add deprecation notice for next `cordova@7` release.
* remove use of `cordovaProject/platforms/platforms.json`
* axe `cordova-lib/src/cordova.platform_metadata.js`
* issue: https://issues.apache.org/jira/browse/CB-13057
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

started the work for removing platform save 13057
apache/cordova-lib#586

This is just a continuation of what we started in `cordova@7`
* **Do Now** deprecate `webos` (in cordova@7)
* Remove `webos`, `bb10`, `ubuntu` + all associated files
* issue: https://issues.apache.org/jira/browse/CB-13056
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

started 13056 --> apache/cordova-lib#588

* https://issues.apache.org/jira/browse/CB-13059

* Remove `cordova platform save` command
This command isn't necessary anymore since we save by default since `cordova@7`. It was previously used to save added plugins and platforms to `config.xml`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would users upgrading from a project using an older cordova version still need this, to save their existing platforms and plugins?
If we remove this, they will have to manually add them in config.xml, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the analytics (assuming I'm doing this correctly), the command has been used 64 times out of the past 1.9 Million requests. Less that 0.1 percent. Pretty clear sign that it is not being used much.

I do see the usefulness of the command for users pre 7 who didn't used to use --save (which has been around much longer). I just don't think people in that group are even aware of the platform save command.

I think by this point asking users to modify config.xml or readd plugin + platforms is fair. They will probably have to readd/update older platforms or plugins anyways.

Removing it will clean up cordova-lib a bit and allow us to get rid of these extra generated config files that go into peoples cordova projects

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into the analytics @stevengill. It is clearly not a use case we need to worry about much, which is good, simplifying the path forward!

@brodycj
Copy link

brodycj commented Jun 3, 2018

Should we merge, close, or do something else with this one?

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.

6 participants