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

Backend Abstraction #67

Merged
merged 31 commits into from Apr 1, 2021
Merged

Conversation

lights0123
Copy link
Contributor

Fixes #66. I'm leaving for a week in 2 days, but I just spent an hour or two working on this today, and I'll probably do a little more tomorrow. Feel free to build on this or just do something else completely.

This introduces a new trait, GenericNode. Right now, it only has one implementation, which is a real DOM node. I'm writing this in a way so that users will have to make their components generic, but will hopefully never have to specify what that generic type is because it'll be inferred from calling render().

Right now, I have the following questions I'll need to answer to go further:

  • Handling next_sibling and parent_node functions. When those just call JS functions, it's no problem because it has a full GC that can handle cyclic references. Although that's possible in Rust with some work, it would be more efficient if we could remove calls to those functions. I could also make those return an error server-side (they're only used for modifying the state), but it would make modifying the data impossible server-side—meaning you can't have some async computation render stuff later, or if you did, it'd have to be implemented differently.
  • NodeRef: How to do this in a user-friendly way: do we force the user to downcast to a concrete type? It would be nice to not hardcode it being a web_sys::Node, for use cases like React Native. However, there is probably no need for it to work on the server.

@lights0123
Copy link
Contributor Author

The basic Hello World demo compiles and works now.

Handling next_sibling and parent_node functions. When those just call JS functions, it's no problem because it has a full GC that can handle cyclic references. Although that's possible in Rust with some work, it would be more efficient if we could remove calls to those functions. I could also make those return an error server-side (they're only used for modifying the state), but it would make modifying the data impossible server-side—meaning you can't have some async computation render stuff later, or if you did, it'd have to be implemented differently.

I decided just to implement them for now. I'll probably make them error out on the server-side.

@lights0123
Copy link
Contributor Author

Hello World, Counter, Components, and the docs all work now. Todomvc doesn't, but that's only because it uses NodeRef. Tomorrow, I'll figure out what to do about that.

@lights0123 lights0123 mentioned this pull request Mar 27, 2021
3 tasks
@lights0123
Copy link
Contributor Author

lights0123 commented Mar 27, 2021

Although, before I do that, does this look good? The alternative way would be with feature flags. That may make NodeRef usage cleaner, but would be less flexible—if, for whatever reason, you'd like the client to be able to render both to the DOM and a string, you'd be prevented from doing so.

Actually, I think I came up with a solution: make NodeRef's get and other functions have a generic parameter which is DOMNode by default. The user can override it if desired for use with an alternate renderer.

Copy link
Collaborator

@lukechu10 lukechu10 left a comment

Choose a reason for hiding this comment

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

Looks good. I think it would make more sense with feature flags because everything that is wasm-bindgen or web-sys would need to be put behind a conditional compilation flag anyways. Otherwise it won't compile on non wasm32 targets.
It would also be nice if we didn't have to make everything generic over G as that is a bit verbose.

The way you implemented this wasn't exactly the way I thought of doing it but that's OK. I was thinking more along the lines of GenericNode having a .to_node() method which would generate DomNodes from the GenericNode. What do you think?

examples/counter/src/main.rs Outdated Show resolved Hide resolved
maple-core/src/internal.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@lukechu10 lukechu10 left a comment

Choose a reason for hiding this comment

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

Sorry I missed some of the new changes in my previous review

maple-core/src/generic_node/vdom.rs Outdated Show resolved Hide resolved
@lukechu10 lukechu10 added C-enhancement Category: new feature or improvement to existing feature A-SSR Area: Server Side Rendering (SSR) and Static Site Generation (SSG) labels Mar 27, 2021
@lights0123
Copy link
Contributor Author

everything that is wasm-bindgen or web-sys would need to be put behind a conditional compilation flag anyways. Otherwise it won't compile on non wasm32 targets.

That's not true—it just panics at runtime.

@lukechu10
Copy link
Collaborator

lukechu10 commented Mar 28, 2021

That's not true—it just panics at runtime.

Yup you're right. My bad. Although I still don't like the generic type params on all the components. Maybe something could be done using dynamic dispatch?

Edit Forget when I said about dynamic dispatch. It doesn't make any sense. I just had an idea. How about making G have a default type of DomNode when a web feature is enabled or SsrNode when a ssr feature is enabled. When both features are enabled, we could either default it to DomNode or not provide a default. What do you think?

@lukechu10
Copy link
Collaborator

Just looked at the ssr example you added and it really is quite impressive! Thanks a lot for all the work so far.

@lukechu10 lukechu10 added this to the 0.5 milestone Mar 28, 2021
@lukechu10
Copy link
Collaborator

About feature flags, it might be a good idea to add a ssr feature anyways to prevent penalizing users who don't use SSR with worse compile times.

@lukechu10 lukechu10 marked this pull request as ready for review March 31, 2021 04:43
@lukechu10 lukechu10 mentioned this pull request Mar 31, 2021
3 tasks
commit 9e4aa92
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Tue Mar 30 15:38:01 2021 -0700

    Separate ssr and dom features

commit 898c27b
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Tue Mar 30 10:24:29 2021 -0700

    Fix intra doc links

commit 6417d75
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Tue Mar 30 09:56:35 2021 -0700

    Move DomNode into submodule

commit 971c776
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Tue Mar 30 09:37:57 2021 -0700

    Rename vdom::Node to ssr_node::SsrNode

commit 1f62d08
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Mon Mar 29 21:34:42 2021 -0700

    Set default type for G depending  on feature

commit cf2b593
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Mon Mar 29 21:19:33 2021 -0700

    Fix clippy issues

commit 74b3975
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Mon Mar 29 16:00:19 2021 -0700

    Add dom and ssr features
commit ace788c
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Tue Mar 30 20:41:48 2021 -0700

    Remove internal module

commit c454e85
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Tue Mar 30 16:03:28 2021 -0700

    Remove internal::attr

commit b0ddb88
Author: Luke Chu <37006668+lukechu10@users.noreply.github.com>
Date:   Tue Mar 30 15:50:08 2021 -0700

    Remove internal::element
maple-core/src/flow.rs Outdated Show resolved Hide resolved
maple-core/src/flow.rs Show resolved Hide resolved
lukechu10 and others added 3 commits April 1, 2021 12:44
Co-authored-by: Ben Schattinger <developer@lights0123.com>
Co-authored-by: Ben Schattinger <developer@lights0123.com>
@lukechu10
Copy link
Collaborator

I'll fix the integration tests and I'll probably merge this. Thanks for all the work @lights0123!

@lukechu10 lukechu10 enabled auto-merge (squash) April 1, 2021 20:36
@lukechu10 lukechu10 merged commit 22909d5 into sycamore-rs:master Apr 1, 2021
@lukechu10 lukechu10 mentioned this pull request Apr 3, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-SSR Area: Server Side Rendering (SSR) and Static Site Generation (SSG) C-enhancement Category: new feature or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce a new abstraction layer in TemplateResult for multiple backends
2 participants