Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Don’t infer project type from the template name #315

Closed
EverlastingBugstopper opened this issue Jul 9, 2019 · 10 comments · Fixed by #759
Closed

Don’t infer project type from the template name #315

EverlastingBugstopper opened this issue Jul 9, 2019 · 10 comments · Fixed by #759
Assignees
Labels
docs refactor regression Something is broken, but works in previous releases
Milestone

Comments

@EverlastingBugstopper
Copy link
Contributor

EverlastingBugstopper commented Jul 9, 2019

Currently in the docs, we suggest that people run wrangler generate myApp https://github.com/cloudflare/rustwasm-worker-template to generate our default rust app. This works because there is a code section that checks to see if rust is in the name of the template, and if it is, it will assume the user wants a project with type=rust. This assumption is shaky and has led to some code sections that are hard to read.

We should stop inferring the project type from the name and either infer from the project structure itself, or require a type to be passed when generating a new project.

There’s a PR for this here: #314 but it breaks one of the commands we suggest on our docs page: wrangler generate myApp https://github.com/cloudflare/rustwasm-worker-template

The reason it breaks is because without a specified type, it defaults to webpack, and our build process makes assumptions about where to look for the javascript, so it can’t build the project w/o the type being set to rust.

This change does work however if you simply run wrangler generate —type rust and is also more succinct than what we currently have in our docs

Blocked by #313 as it makes changes to this particular section of code

@xortive
Copy link
Contributor

xortive commented Jul 9, 2019

We can just include a mostly-empty wrangler.toml that has type set correctly for each project template

@EverlastingBugstopper
Copy link
Contributor Author

EverlastingBugstopper commented Jul 10, 2019

Problem

When creating a new project with wrangler generate there are a couple of cases we need to cover. Sometimes —type is passed + —template isn't -- we want to provide a template that works with the type they pass in (see #309). Sometimes —template is passed in and not —type. These are the types of commands we recommend on workers.cloudflare.com/docs. For this case, we want to be able to infer the type of the project based on the template.

Proposed solution

There’s not really an easy way to infer the type from the template unless there is something in the template itself that hints at what In our templates, we can provide a mostly empty wrangler.toml that is identical to the desired wrangler.toml we would want wrangler to generate for the project anyway. This allows wrangler to have confidence that the type it is assigning to the template is correct + that it can build it.

Required changes

This will require PRs to add a wrangler.toml to each of the template repositories, and changes to wrangler to read the type from the template’s wrangler.toml in addition to modifying the name of the template when a name is passed from the command line.

Open questions

  • User specifies a template without a wrangler.toml
    • Do we bail and ask the user to pass the type or guess that it’s webpack and print a warning?
  • User specifies a type (say rust) and a template with a different type (say webpack)
    • What type do we use? I’m thinking we use the one provided by the user but print a warning

@ashleymichal
Copy link
Contributor

ashleymichal commented Jul 11, 2019

scoping: implementing the proposed solution will involve writing logic to alter an existing wrangler.toml in order to add the project name.

@EverlastingBugstopper
Copy link
Contributor Author

Could you explain what you mean by that/why? I don’t think this solution requires any changes to the .toml itself.

@ashleymichal
Copy link
Contributor

ashleymichal commented Jul 11, 2019

during the generate phase, wrangler actually generates a toml on behalf of the user, and it uses the name argument passed by the user for the project name field. so if you run wrangler generate my-specific-use-case <someGenericTemplate> you get a toml with name my-specific-use-case

@xortive
Copy link
Contributor

xortive commented Jul 11, 2019

Serde should handle this for us -- we serialize the config, change it, then deserialize

@EverlastingBugstopper
Copy link
Contributor Author

Ah! I understand now :) Awesome. @granjef3’s comment makes sense. serde is pretty magic + what we use to write to the .toml currently. Should be fairly easy to include that. I’ll edit my proposed solution

@xortive
Copy link
Contributor

xortive commented Jul 12, 2019 via email

@EverlastingBugstopper
Copy link
Contributor Author

Work that needs to be done

  1. Add a type field to all of our template's wrangler.toml files
  2. When template is specified and not type in wrangler generate, de-serialize the template's wrangler.toml and use its type field in the new wrangler.toml that wrangler creates.

Open questions:
How should wrangler behave when both a type and a template is specified in a wrangler generate command, and a type is also in the template's wrangler.toml? Should we use the type that the user passed to us? Or should we take the type from the wrangler.toml in the template?

How should wrangler handle generate commands where a template without a type field in wrangler.toml is used, and a type argument is not passed inline? Should it just default to webpack? Maybe warn the user?

@ashleymichal
Copy link
Contributor

ashleymichal commented Aug 30, 2019

this is dependent on updating all the templates above; may not make it in 1.3.0. It may also be wise to let the Environments milestone soak for a bit before implementing this change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
docs refactor regression Something is broken, but works in previous releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants