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

Rethink templating #37

Closed
sebinsua opened this issue Aug 5, 2020 · 7 comments
Closed

Rethink templating #37

sebinsua opened this issue Aug 5, 2020 · 7 comments

Comments

@sebinsua
Copy link
Contributor

sebinsua commented Aug 5, 2020

I mentioned to @NMinhNguyen that I had a few misgiving when adding a templating feature.

  1. Partly, the implementation is a little confusing right now due to the way we are piggy-backing on the installation of create-react-app. There are changes that create-react-app makes which are not part of its template and we then need to undo or amend. It'd be nicer if everything was just copied across from a single folder.

  2. More substantively, a widget is dependent on an app, so if the configuration of an app and verson of modular installed doesn't include support for TypeScript you should not be able to install a widget that depends on this. I'm not suggesting that we add validation -- probably it means that the template for a widget is stored within the modular package (for now called modular-scripts). This was brought up by @threepointone a week or so ago, and I'm writing it down now for posterity.

  3. Finally, there are now three templates (repo, app and widget). Currently, only app and widget are configurable, and we will likely want repo to be configurable, too. (If/when this is required, it might need to support something like wizards.)

@threepointone
Copy link
Contributor

I'm happy to consider not having templates at all, some thoughts -

  • one thing I assumed we'd need templates for, are if people want to use regular js instead of ts. If people can write regular js anyway inside our current default template, this is a non-issue. I don't think anyone cares particularly about flow in open source/jpm (ie - outside fb)
  • multi platform support: we may want to setup for react native in the future; but maybe it makes more sense to make a separate modular-native package then.
  • we may want a flow for adding different types of widgets; this is also not super important, since people could literally copy paste snippets/dependency lists from somewhere.

So... yeah. Maybe we shouldn't have templates? We could revisit this in the far future and add templating support when actual usecases arise (but it's harder to go the other way; adding template support now and pulling back later).

@sebinsua
Copy link
Contributor Author

sebinsua commented Aug 5, 2020

Ha, you were the one that asked for them originally! ;D

But, yes, I agree that we could avoid them entirely, and only add them back in if they're required. Or rather leave the code, but remove the --template argument, since that is faster.

I think the original desire for them was to add support for 'configuring CI on a repo' however if our aim is to avoid people creating lots of separate repos why try to simplify or automate a step that we want to happen as little as possible? Also, if that were required, perhaps a separate tool could be provided to do this.

(Other stuff as you mention like the ability to use regular JS still works with this setup. And the need to add a different kind of widget is not of utmost importance right now.)

@threepointone
Copy link
Contributor

Ha, you were the one that asked for them originally! ;D

I'm also a big fan of changing my mind when presented with new information :P

Yeah, let's just remove the template arg for now (happy to do it myself too if you'd like, I could send it as soon as tonight)

@threepointone threepointone mentioned this issue Aug 5, 2020
7 tasks
threepointone added a commit that referenced this issue Aug 6, 2020
ref: #37

adding support for templates makes this implentation quite complex, and we're not solving an exisitng problem atm. Instead, we'll use the default template for now, and revisit when an actual need comes up. More context in the linked issue.
threepointone added a commit that referenced this issue Aug 6, 2020
ref: #37

adding support for templates makes this implentation quite complex, and we're not solving an exisitng problem atm. Instead, we'll use the default template for now, and revisit when an actual need comes up. More context in the linked issue.
threepointone added a commit that referenced this issue Aug 6, 2020
ref: #37

adding support for templates makes this implentation quite complex, and we're not solving an exisitng problem atm. Instead, we'll use the default template for now, and revisit when an actual need comes up. More context in the linked issue.
@threepointone
Copy link
Contributor

Leaving this open for a bit, will close by the end of the week if no new info presents itself.

@threepointone
Copy link
Contributor

Closing this.

@threepointone
Copy link
Contributor

Reopening this, since I think we now have some usecases popping up. Not for the main starting template, but for widgets. It remains to be seen whether this needs to be a --template option, or whether we ask questions after modular add <package> and then either change just some portions of one template, or load one of a couple of blessed templates, or whatever it may be.

@threepointone threepointone reopened this Sep 1, 2020
threepointone added a commit that referenced this issue Sep 3, 2020
As prep for hosting multiple apps, this PR changes the commands from:
```
modular build
modular start
```
to
```
modular build <path>
modular start <path>
```

In the next PR, I'll change 'modular add' to ask a question to the user, which would then generate either an app, a widget, or a regular package. Combining the 2 PRs, we can then host multiple apps as described in #67, and possibly answer #37 too.
threepointone added a commit that referenced this issue Sep 3, 2020
As prep for hosting multiple apps, this PR changes the commands from:
```
modular build
modular start
```
to
```
modular build <path>
modular start <path>
```

In the next PR, I'll change 'modular add' to ask a question to the user, which would then generate either an app, a widget, or a regular package. Combining the 2 PRs, we can then host multiple apps as described in #67, and possibly answer #37 too.
threepointone added a commit that referenced this issue Sep 4, 2020
As prep for hosting multiple apps, this PR changes the commands from:
```
modular build
modular start
```
to
```
modular build <path>
modular start <path>
```

In the next PR, I'll change 'modular add' to ask a question to the user, which would then generate either an app, a widget, or a regular package. Combining the 2 PRs, we can then host multiple apps as described in #67, and possibly answer #37 too.
@threepointone
Copy link
Contributor

Took a call with this and we now have 3 'types' of packages, and it's still not something you can pass to the tool. Closing this again.

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

No branches or pull requests

2 participants