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

Builder API #269

Merged
merged 29 commits into from Nov 5, 2021
Merged

Builder API #269

merged 29 commits into from Nov 5, 2021

Conversation

jquesada2016
Copy link
Contributor

I have not fully tested, nor do I think the current iteration has the right aesthetics, but I believe it's in the right direction! This is an implementation attempt for the builder API as mentioned in #268.

@jquesada2016
Copy link
Contributor Author

@lukechu10 All tests should pass. I went through the logs and adjusted some code to run on rustc v1.53, but it seems the tests did not re-run. Not sure why. Also, API is stable enough for an initial audit. I've migrated our project to use the API as it currently stands and have no issues. Parts that remain untested are the styles functions, since we aren't using styles. I will also be adding docs to the functions.

@lukechu10
Copy link
Collaborator

Wow thanks for all the work. I'll try to get to this soon.

@jquesada2016
Copy link
Contributor Author

Would you like to try to aim this feature for v0.7.0?

@lukechu10
Copy link
Collaborator

lukechu10 commented Sep 30, 2021

Yes. That sounds good. But no pressure though, that will take some time.

@lukechu10 lukechu10 self-requested a review October 9, 2021 17:07
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.

Thank you for all the work you put in and sorry for taking so long to review it :)

This looks really great!

packages/sycamore/src/builder/agnostic/mod.rs Outdated Show resolved Hide resolved
packages/sycamore/src/builder/agnostic/mod.rs Outdated Show resolved Hide resolved
packages/sycamore/src/builder/agnostic/mod.rs Show resolved Hide resolved
packages/sycamore/src/builder/agnostic/mod.rs Outdated Show resolved Hide resolved
packages/sycamore/src/builder/agnostic/mod.rs Outdated Show resolved Hide resolved
Comment on lines 530 to 548
/// Binds `sub` to the `value` property of the node.
///
/// Note that this is the same as calling `.dyn_prop("value", sub)`.
///
/// # Example
/// ```
/// # use sycamore::prelude::*;
///
/// # fn _test<G: GenericNode>() -> Template<G> {
/// let value = Signal::new(String::new());
///
/// input()
/// .bind_value(value)
/// .build()
/// # }
/// ```
pub fn bind_value(&self, sub: Signal<String>) -> &Self {
let sub_handle = create_memo(cloned!((sub) => move || {
Some((*sub.get()).clone())
Copy link
Collaborator

@lukechu10 lukechu10 Oct 9, 2021

Choose a reason for hiding this comment

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

(This comment applies to both the bind_value and bind_checked function. I couldn't select the whole thing on GitHub)

2 things:

  • This should probably belong inside the html builder since it is html specific.
  • Instead of having bind_value and bind_checked, maybe a more generic bind_str method that accepts an event_name: &str and sub: Signal<String> and equivalent bind_bool method.
    event_name would thus be value or checked.

packages/sycamore/src/generic_node/ssr_node.rs Outdated Show resolved Hide resolved
packages/sycamore/src/lib.rs Outdated Show resolved Hide resolved
packages/sycamore/src/builder/html.rs Outdated Show resolved Hide resolved
@jquesada2016
Copy link
Contributor Author

No problem! I'm going to be mostly MIA the next couple weeks due to traveling. But I'll try to work on this whenever I get a chance.

Is there a reas-on why we need to pull in macro_rules_attribute? Sure, it looks slightly nicer than doing

What I was thinking was eventually making this a public macro which users could use for extending the builder API to use custom typed tags, which would be very useful for quickly adding support for something like Svelte Native, along with web components UI frameworks. Also, since this is a very simple macro, it didn't merit, in my opinion, dropping to a full procedural macro.

I would prefer not adding all the html tags into the scope by default.
Sure! No problem by me. I'll just keep in the html macro to the prelude by default.

.intersperse() is nightly. Stable unfortunately needs an extra allocation.

huh...I remember fixing this...

I would prefer not to add this because it promotes bad code architecture. Generally, it is much clearer to dispatch inside the event handler instead of making the event handler itself dynamic.

Agreed.

Maybe something shorter like on would be better. What do you think?

I do like on more!

I would prefer not to add these functions because they can easily be mal-used and break the internal logic of Sycamore.

Sure, we can remove these, though I can imagine some workflows, especially with dynamic tables or virtual lists where this kind of optimization could be useful. Perhaps mark the function as unsafe? (though this might break the builder flow) If not, I'm fine with removing it.

@lukechu10
Copy link
Collaborator

No problem! I'm going to be mostly MIA the next couple weeks due to traveling. But I'll try to work on this whenever I get a chance.

No worries!

What I was thinking was eventually making this a public macro which users could use for extending the builder API to use custom typed tags, which would be very useful for quickly adding support for something like Svelte Native, along with web components UI frameworks. Also, since this is a very simple macro, it didn't merit, in my opinion, dropping to a full procedural macro.

What I meant was

generate_tag_functions! { enum HtmlTag { ... } }

instead of

#[apply(generate_tag_functions)]
enum HtmlTag { ... }

because they are equivalent.

Sure, we can remove these, though I can imagine some workflows, especially with dynamic tables or virtual lists where this kind of optimization could be useful. Perhaps mark the function as unsafe? (though this might break the builder flow) If not, I'm fine with removing it.

We might be able to do something about this with type-states but I think that belongs in another PR.

@lukechu10 lukechu10 added A-builder Area: builder API C-enhancement Category: new feature or improvement to existing feature labels Oct 16, 2021
@lukechu10
Copy link
Collaborator

If you don't mind, I can merge this now and fix the issues I mentioned in a later PR to not make this one too long.

@jquesada2016
Copy link
Contributor Author

jquesada2016 commented Oct 21, 2021 via email

@lukechu10 lukechu10 merged commit 924ed0c into sycamore-rs:master Nov 5, 2021
This was referenced Nov 5, 2021
@ctjhoa ctjhoa mentioned this pull request Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builder Area: builder API C-enhancement Category: new feature or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants