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

Extract project structure into its own file #977

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

janpio
Copy link
Member

@janpio janpio commented Apr 28, 2019

This file is extraxcted from cordova-cli via apache/cordova-cli#428

Slight modifications already done, will have to checked if still up to date and probably restructured quite a bit.

Comments and suggestions welcome!


### Version control

It is recommended not to check in `platforms/` and `plugins/` directories into version control as they are considered a build artifact. Your platforms and plugins will be saved in config.xml & package.json automatically. These platforms/plugins will be downloaded when `cordova prepare` is invoked.
Copy link
Member

Choose a reason for hiding this comment

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

This should say "Your platforms and plugins will be saved in package.json automatically."

We no longer save them into config.xml as of Cordova 9.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, exactly why I created this PR. This information being buried in the CLI referenc means it is ignored and I think most users never have seen it - but it could be so useful to understand what Cordova is actually doing and how it works.

I would love if you could add more comment on inaccuracies or missing information of this type here. Will try to work it all in.

Copy link
Member

Choose a reason for hiding this comment

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

I applied changes so this can be re-reviewed.

|-- package.json
|-- config.xml
|-- hooks/
|-- merges/
Copy link
Member

Choose a reason for hiding this comment

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

The merges folder is not created automatically anymore, and we kinda discourage its use (although it's still supported as far as I know).

Also, the example folders here are indented one level too far.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is what is written below about merges still accurate though?
Will have to splti existing document into "created by default" and "also supported" somehow I guess.

Copy link
Member

@erisu erisu Jul 27, 2022

Choose a reason for hiding this comment

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

I applied changes so this can be re-reviewed.

I tried to reorg the structure so there are now the "default" and "optional" sections.

Also, the example folders here are indented one level too far.

As for this comment, I am not sure what was wrong with the indention level. I went ahead and replaced the entire content with the printout of the tree command.

I made little edits to the printout such as:

  • Add the myapp to the top.
  • Add a / to the end of directories. This should remove confusion between a directory vs file with no extension.

myapp/
|-- package.json
|-- config.xml
|-- hooks/
Copy link
Contributor

Choose a reason for hiding this comment

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

As with merges, usage of the hooks folder is deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hooks are still supported via config files though, and then can be in random locations, right?

So this here should be removed, but maybe a new paragraph that links to the hooks documentation (that hopyfully exists).

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Hooks can live anywhere, and we want them explicitly listed in config.xml now instead of auto-detected through the hooks folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

For later reference: https://cordova.apache.org/docs/en/9.x/guide/appdev/hooks/index.html is the docs about hooks

Copy link
Member

Choose a reason for hiding this comment

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

I applied changes so this can be re-reviewed.

@erisu erisu force-pushed the janpio-extract_project_structure_from_cli branch from 1a3a19e to be72066 Compare July 27, 2022 03:20
@erisu erisu requested review from dpogue and raphinesse July 27, 2022 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants