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

New Multiplatform Templates #50

Open
wants to merge 31 commits into
base: v0.6
Choose a base branch
from

Conversation

DogeDark
Copy link
Member

No description provided.

@Andrew15-5
Copy link
Contributor

I'll link this just in case: ealmloff#2 (comment).

@DogeDark DogeDark marked this pull request as ready for review November 18, 2024 22:13
@DogeDark
Copy link
Member Author

The templates are set to use alpha 5 but can easily be changed at the top of the init-common.rhai script.

@Andrew15-5
Copy link
Contributor

Andrew15-5 commented Nov 19, 2024

The .git/HEAD file shouldn't show the name of the branch used to create the project with dx new this -t .../dioxus-template. Is this cargo-generate's issue?

Shoudn't the Barebones template spelled as Bare-bones instead?

(Barebones/Jumpstart) Why I can't choose Mobile platform to serve by default? If I only choose Mobile, then it is set as default platform/feature, but the README doesn't say that I have to add --platform android etc. unlike in the Workspace template. So the dx serve in this case errors.

Also the 0.6 alpha version is kinda strange, since the upstream repo should either have a 0.5 until the 0.6 release, or 0.6 when it released. But maybe if it's very quick, then that would be OK.

I haven't compiled anything yet though.

@Andrew15-5
Copy link
Contributor

Andrew15-5 commented Nov 19, 2024

Speaking of dark theme, there is a light-dark function now! I hope it will work as expected, because the browsers support it, but I don't know about WebView libs.

https://www.youtube.com/watch?v=A89FMtIkWKc
https://caniuse.com/?search=light-dark

@DogeDark
Copy link
Member Author

@Andrew15-5

I believe the .git/HEAD thing is cargo-generate - we don't do any git stuff in the templates.

I suppose it should be Bare-Bones

Dioxus can't currently serve mobile by default, I'll add some more instructions to those readmes for that.

With the version, if you're talking about the template, they are now versioned by branch. This PR is merging into dioxus-template/0.6 and shouldn't be 0.5.

@jkelleyrtp jkelleyrtp self-requested a review November 20, 2024 13:39
Copy link
Member

@jkelleyrtp jkelleyrtp left a comment

Choose a reason for hiding this comment

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

Some thoughts:

The Calculator demo is nice insofar as it demonstrates how fullstack works, but I think the Calculator component is too complex for a template. I think something simpler (no validation, smaller component) like an "echo" component that simply echos and/or modifies your input would be better, at least for the simpler templates.

This is just a taste thing but the logging situation should also be simplified - I would be fine with no explicit info!() call - maybe we can put an info!() call in our dioxus::launch function instead. In dioxus v0.7 I'd like to wrap in dioxus_logger as dioxus::logger and have the init be automatic in launch.

fn main() {
    dioxus_logger::init(Level::INFO);
    dioxus::launch(App);
}

Also now that the templates are quite complex, do you think we can set up a github workflow that tests each out with a simple shell script?

  • run dx new <path> --subtemplate XYZ --path XYZ
  • cargo dx check and deny warnings

@DogeDark
Copy link
Member Author

@jkelleyrtp Regarding the info!() call, usually I wouldn't have it at all. I included it here as a subtle kind of tutorial, like "hey, this is how you can log stuff" but I suppose that's what the docs and guide are for - I will remove it.

@DogeDark
Copy link
Member Author

DogeDark commented Nov 21, 2024

  • Update readmes with some extra mobile docs
  • Remove info! calls
  • Replace calculator with echo
  • CI Tests

@DogeDark
Copy link
Member Author

Unfortunately due to the heavy use of rhai scripts, I don't think the --option x=y (dx) aka --define x=y (cargo-generate) works anymore.

Tried checking the variable in the rhai script to see if it was unset but cargo-generate doesn't seem to return anything useful for that purpose; even if it is set with --define. I imagine this is because of the script order in relation to cargo-generate itself but we have to run in the init phase.

@Andrew15-5
Copy link
Contributor

I tried this too and remembered your comment. Now --yes doesn't work which removes the ability to create a Dioxus project without any interaction. What was the reason in using rhai in the first place? Is cargo-generate.toml that limiting? And the subtemplates are still there: #42.

@jkelleyrtp
Copy link
Member

We should simplify the template or get the --yes working again - would be ideal for harness based end to end testing.

@Andrew15-5
Copy link
Contributor

Andrew15-5 commented Nov 24, 2024

In fact, it already (if this PR is merged to main) will make my tests fail (DioxusLabs/dioxus#2822), since they depend on --yes:

image

@DogeDark
Copy link
Member Author

DogeDark commented Nov 24, 2024

What was the reason in using rhai in the first place?

Questions based on the answer of previous questions. E.g. the dynamic list of platforms for the "Which default platform?" question. I decided to then use rhai for everything as it felt messy having questions for cargo-generate and then questions asked in rhai and the whole ordering of questions would be messed up.

I'll play around with the script ordering and see if I can come up with anything, but this feels like something that should be improved in cargo-generate itself. I had to use the Linux expect cli to run the CI tests here.

@Andrew15-5
Copy link
Contributor

Thanks. We all know that cargo-generate isn't ideal, but I guess it's our best bet for now. I could contribute to it to make it more flexible, but I don't know if I actually can add new features. It will probably take a while for all the discussions.

Can't we have a bit simpler logic for now? Is there any other way the default platform can be selected correctly? I think if this is the only blocker, then it's probably not that bad, if we outsource this task to the end user via the README.md file. Like add a line "in ... write your preferred default platform".

@DogeDark
Copy link
Member Author

DogeDark commented Nov 24, 2024

I just tested it and it looks like pre scripts are run before template expansion, and they get any variables defined through --define! We need logic to prevent the scripts from asking for already-set values; otherwise, we should be good.

New issue: Values provided through --define are always strings and the variable's type can't be overridden so the rest of the template fails. I think I can get around this though.

@DogeDark
Copy link
Member Author

DogeDark commented Nov 25, 2024

This can now work with --yes and --option but the --yes will still need --option default_platform=xyz to be set for the sub-templates that need it. Unless we remove the default platform stuff entirely, but that means it's no longer "run-and-done"

The CLI will also need a fix as it currently errors on the Workspace template (most likely because it expects a file that doesn't exist like it does on the other sub-templates)

@Andrew15-5
Copy link
Contributor

I'm not sure if the "run-and-done" must be implement by 0.6. I think for now CI/tests are more important, so the noninteractive project creation should work without additional prompts. We can cite on cargo-generate being not yet flexible enough for our use case.

I have a question: shouldn't the web platform be default usually and for now? Does it have a separate feature? I have a feeling that previously it worked without any additional feature, so nothing will break if it is always the default. The user later can change the default platform if web isn't preferred.

@DogeDark
Copy link
Member Author

Yes, web has it's own feature: dioxus/web. The template generates all the features for the selected platforms and we need to specify a default for dx to work without the --platform flag.

While we could default to the web platform, I think the current implementation is a good compromise between --yes and normal use. The majority of users are going to use the prompts and I feel providing default platform selection is more valuable than removing the extra flag required for --yes. This flag could be automatically added to default to the web platform in the CLI to negate this issue.

I'll leave the decision up to @jkelleyrtp.

@Andrew15-5
Copy link
Contributor

This flag could be automatically added to default to the web platform in the CLI to negate this issue.

In the CLI, you mean hard-code in the create/init functions? So that you can again just add --yes and that's it? Hard-coding such stuff is wrong, because this can break some other templates that the users can use.

I think we still have the problem of not being able to use the default subtemplate because of cargo-generate/cargo-generate#1181. So ideally it would automatically choose the Bare-Bones subtemplate.

BTW, that issue is now so old and was mentioned quite a few times, I really want to fix it already and forget about that. But I'm really busy now (still). Maybe in December I'll have some free time for this.

@DogeDark
Copy link
Member Author

because this can break some other templates that the users can use.

It can check if it's using these specific templates.

@Andrew15-5
Copy link
Contributor

Well, yeah, I guess that can be checked. This still feels wrong, but at least now there will be no harm in doing that. We just really should hope that it's actually temporary, otherwise it will be a permanent fix.

@Andrew15-5
Copy link
Contributor

I noticed that even though I included Tailwind, it doesn't work. Then I realized that this is because I don't include the CSS file into the HTML head.

The readme should be changed, so that it also says ./assets/styling/tailwind.css same as all other assets, otherwise the CSS asset path will be inconsistent:

const FAVICON: Asset = asset!("/assets/favicon.ico");
const MAIN_CSS: Asset = asset!("/assets/styling/main.css");
const TAILWIND_CSS: Asset = asset!("/assets/tailwind.css");

It's also strange that Tailwind hot-reloading doesn't work, at least with desktop. I thought both CSS and Tailwind are fixed already. I have to rebuild the app so that it can use the newly generated Tailwind classes.

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.

4 participants