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

feat: viz integration #506

Merged
merged 3 commits into from
Feb 21, 2023
Merged

feat: viz integration #506

merged 3 commits into from
Feb 21, 2023

Conversation

fundon
Copy link
Contributor

@fundon fundon commented Feb 12, 2023

Viz is a fast, flexible, lightweight web framework.

Repo: https://github.com/viz-rs/viz
Website: https://viz.rs/

  • leptos-viz
  • examples

If you don't think it makes sense to put it in the leptos project, you can close it. :)

@fundon fundon changed the title [wip] feat: integrate viz [wip] feat: viz integration Feb 12, 2023
@gbj
Copy link
Collaborator

gbj commented Feb 12, 2023

Cool! Are you still working on the integration or is this ready to review?

EDIT: Never mind you titled it WIP haha. Just let me know when it's ready.

@fundon fundon changed the title [wip] feat: viz integration feat: viz integration Feb 14, 2023
@fundon
Copy link
Contributor Author

fundon commented Feb 14, 2023

I'm done updating and ready to review.

@gbj
Copy link
Collaborator

gbj commented Feb 15, 2023

Thanks for your work on this. I'm not very familiar with Viz, but it looks like the internals are very similar to Axum... diffing this against the Axum integration it looks like there's a lot of shared code that could probably do with a refactor. Same goes for the examples, of course.

I'm working on another PR that is already doing some refactoring of the integrations, so I'm happy to take this as is for now, and handle the refactoring as part of that other work.

On the examples, on the other hand — rather than duplicating three existing examples, could you please pick one to highlight Viz and remove the other two? This will reduce the maintenance burden for me over time if and when we need to update the application code in examples.

@fundon
Copy link
Contributor Author

fundon commented Feb 15, 2023

Thank you for your reply.

Yes, they are somewhat similar. We can extract some common functions.

I'll wait for you to finish that PR and refactor this one.

It feels like we can just keep the sqlite example.

@gbj
Copy link
Collaborator

gbj commented Feb 18, 2023

@fundon I've merged the other PR — So if you compare what you did for leptos_viz to the version of leptos_axum that's in main now, you should be able to apply more or less the same set of changes you did earlier, and get support for in-order streaming and async rendering out of the box. I wasn't able to refactor quite as much as I'd hoped but every bit counts. Let me know if you run into any issues and I hope this doesn't cause you to duplicate too much of your work.

@fundon fundon force-pushed the integrate-viz branch 3 times, most recently from 979b3f9 to c9dbfc5 Compare February 19, 2023 07:29
@fundon
Copy link
Contributor Author

fundon commented Feb 19, 2023

@gbj I have merged the latest changes and only keep the sqlite example.

If you come across viz related later, you can tell me and I can maintain this part of the code. :)

@gbj
Copy link
Collaborator

gbj commented Feb 19, 2023

Thanks! This is great.

The doctests are failing on

---- src/lib.rs - render_app_async (line 746) stdout ----
error[E0423]: expected function, tuple struct or tuple variant, found struct `ServiceMaker`
  --> src/lib.rs:776:16
   |
33 |         .serve(ServiceMaker(app))
   |                ^^^^^^^^^^^^^^^^^ help: use struct literal syntax instead: `ServiceMaker { tree: val }`
   |
  ::: /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/viz-0.4.8/src/server/service.rs:18:1
   |
18 | pub struct ServiceMaker {
   | ----------------------- `ServiceMaker` defined here

@fundon
Copy link
Contributor Author

fundon commented Feb 21, 2023

@gbj It has been fixed.

@gbj gbj merged commit e9c4b49 into leptos-rs:main Feb 21, 2023
@gbj
Copy link
Collaborator

gbj commented Feb 21, 2023

Thanks for your work! I've merged this. I'll release a leptos_viz package as part of our little world of packages during the next release cycle, if that's all right with you.

@fundon fundon deleted the integrate-viz branch February 22, 2023 04:00
gbj pushed a commit that referenced this pull request Mar 21, 2023
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.

None yet

2 participants