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

Change leptos_axum generate_route_list function to not be async #1485

Merged
merged 7 commits into from
Sep 22, 2023

Conversation

nicoburniske
Copy link
Contributor

Closes #1392

@gbj gbj added this to the 0.5.0 milestone Aug 4, 2023
@gbj
Copy link
Collaborator

gbj commented Aug 24, 2023

Could you remind me of the reason you want it to be sync?

My thinking is this:

  • It's already marked async, so removing that is a breaking change that will affect all applications
  • It doesn't currently need async, but being marked async means that it can use async in the future
  • There are possible situations in the future when we might want it to be async. For example, people have been talking about static site generation for a long time and it might eventually be useful to build in support for that, building on generate_route_list. For example, "when I run this CLI tool I want to build static HTML versions of every page. To figure out the list of pages, I need to query the DB for blog posts." Having the flexibility to use async APIs here is useful.

So what's the use case that you need it not to be async for? Then we can weigh the pros and cons.

@rakshith-ravi
Copy link
Collaborator

Random joke. Ignore me

It doesn't currently need async, but being marked async means that it can use async in the future

The fact that it's async means that it already uses a Future.
Okay I'll show myself out

@gbj
Copy link
Collaborator

gbj commented Sep 8, 2023

So, this PR (#1649) is a great example of why I want to keep it async, at least for now; it builds in some static-site-generation features, which rely on async, and uses generate_route_list.

@gbj
Copy link
Collaborator

gbj commented Sep 15, 2023

Okay I've had a change of heart once I've looked at it in more depth, and in the context of #1649 as well (which no longer requires async route generation in this case). I think this is a good idea for consistency with the leptos_actix integration, which is not async here.

@mrvillage
Copy link
Contributor

If axum is going to change, would it not be prudent to also make viz's function non-async?

@gbj gbj merged commit 38d1727 into leptos-rs:main Sep 22, 2023
62 checks passed
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.

Rework leptos_axum generate_routes() to not be async
4 participants