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 new template proposal #49

Merged
merged 3 commits into from
Jul 20, 2016
Merged

Conversation

stevengill
Copy link
Contributor

No description provided.

@stevengill
Copy link
Contributor Author

@csantanapr thoughts?

@stevengill
Copy link
Contributor Author

So I was chatting with @carynbear about this proposal.

Some things we should chat about adding to this proposal:

@stevengill
Copy link
Contributor Author

@nikhilkh @purplecabbage @vladimir-kotikov please take a look when you have time.

@ryanjsalva
Copy link
Contributor

I'm sorry, I don't understand why we wouldn't want to copy over package.json. It seems like you would want package.json the majority -- not the minority -- of the time. Can you help me understand why we're special casing it here? It seems like we have the potential to create more confusion by adding a set of hidden exceptions.

@csantanapr
Copy link
Member

Steve LGTM it covers what we discussed on files to ignore that are not for the template and are part of delivery in npm

Also the name and version fields in package.json like config.xml it's a new idea I like it +1

@purplecabbage
Copy link
Contributor

Hi Ryan,
The reasoning here is that in a template repo, there are some things that belong to the template-repo and there are some things that belong to the application that is created from the template.

ie.
template-repo/.git/ <= this would be expected to be part of .gitignore, but if installed from a local fork, we still have an issue.
template-repo/README.md
template-repo/package.json
template-repo/template-src/package.json

Ultimately here, I think the issue is that our template logic is doing a blind copy of everything. I think the target template itself should define what is part of the template, and what is not.

@purplecabbage
risingj.com

On Fri, Jun 10, 2016 at 7:24 PM, Carlos Santana notifications@github.com wrote:
Steve LGTM it covers what we discussed on files to ignore that are not for the template and are part of delivery in npm

Also the name and version fields in package.json like config.xml it's a new idea I like it +1


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@stevengill
Copy link
Contributor Author

@ryanjsalva we definitely want package.json. The issue is we don't want to copy over the package.json being used to publish a template to npm since it will have incorrect values for name, version, keywords, author, dependencies, etc. We will copy package.json (and everything else) from a template defined subdirectory.

@stevengill
Copy link
Contributor Author

Here is a PR by @carynbear enhancing template support.

Things to note:

  • --template now skips copying the templates package.json as well as other files mentioned in proposal.
  • if a template defines a subdirectory, it will copy everything from that sub directory (including package.json)
  • if a package.json is included in the subdirectory, it will be copied and updated. Specifically, package.name will be a lowercased version of the third argument cordova create takes (which is the human readable name) and package.version will be set to 1.0.0
  • the version in config.xml will also be set to 1.0.0
  • Removed symlinking code. This means --link-to won't be supported in cordova-create anymore. This didn't work well on Windows since it required administrative privileges. The code for it was very ugly and IMO not very maintainable. The usecase we lose is symlinking a www directory to your cordova app. I believe @carynbear has a branch where she has kept the symlinking code intact. What are your thoughts on this?
  • --fetch is accepted when doing cordova create. This will use cordova-fetch to grab module/repo and fetch it to your ~/.cordova/node_modules directory to be copied over.

Please review it @csantanapr @nikhilkh @purplecabbage

Anything to add @carynbear?

@stevengill
Copy link
Contributor Author

Oops, forgot to link to the PR. apache/cordova-lib#456

@stevengill
Copy link
Contributor Author

Going to merge this in today

@purplecabbage
Copy link
Contributor

Wait please.

@stevengill
Copy link
Contributor Author

Merging in now!

@vladimir-kotikov
Copy link
Member

Hey @stevengill, @carynbear - there is some failures (see here), caused by the change in apache/cordova-lib#456. Looks like --copy-from option in 'createmobilespec' is being ignored. Could you please confirm if this is just a bug or --copy-from is not supported anymore?

@stevengill
Copy link
Contributor Author

Thanks @vladimir-kotikov. I'll take a look

@stevengill
Copy link
Contributor Author

@vladimir-kotikov I pushed a fix to createmobilespec.js. it was using --link-to which we removed. I updated it to use --template

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.

5 participants