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

Rewrite all the dioxus_elements logic for more modular hotreload support #3318

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rambip
Copy link

@rambip rambip commented Dec 9, 2024

It is very much a draft.
For now, this channel is mainly a way for me to write down my reasoning about what has to be done.

The final goal of this PR is to get to a state where:

  • the knowledge about the elements is not baked into the cli
  • the user can easily add or change (and maybe delete) the elements we wants to use in the rsx
  • the rsx hotreload and the compilation does not take more time
  • maybe use a trait-based element definition to allow the user to add it's own element without having dioxus_elements in scope
  • the code for hotreload is simpler (no HotReloadCtx trait)

I also have a different PR #3274 that generates the elements spec with codegen, and this work is almost usable right now. I will have to merge the 2 at one point.

@rambip
Copy link
Author

rambip commented Dec 9, 2024

ROADMAP:

@rambip rambip reopened this Dec 9, 2024
@rambip
Copy link
Author

rambip commented Dec 11, 2024

First difficulty: where must the element namespace be static, and when does it have to do dynamic ?
Clearly, since the cli reads the element specification at runtime, it's element specifications are dynamic.

Right now, the fileds of TemplateNode for tag and namespace are static, and I think it will not work for our approach.
We could Box::leak it but it's not very elegant.

@rambip
Copy link
Author

rambip commented Dec 11, 2024

Right now, the hotreloading is enabled by the following trait (https://github.com/DioxusLabs/dioxus/blob/main/packages/core-types/src/hr_context.rs):

pub trait HotReloadingContext {
    fn map_attribute(
        element_name_rust: &str,
        attribute_name_rust: &str,
    ) -> Option<(&'static str, Option<&'static str>)>;
    fn map_element(element_name_rust: &str) -> Option<(&'static str, Option<&'static str>)>;
}

Meaning that the namespace of the attribute depends on the parent element. If we have to call manganis::register_attribute for each attribute inside each element, the amount of static data generated will be huge.

I have to find a way to compress it, like pointing to an attribute inside the definition of an element

@ealmloff
Copy link
Member

First difficulty: where must the element namespace be static, and when does it have to do dynamic ? Clearly, since the cli reads the element specification at runtime, it's element specifications are dynamic.

Right now, the fields of TemplateNode for tag and namespace are static, and I think it will not work for our approach. We could Box::leak it but it's not very elegant.

We currently leak the string in debug mode during hot reloading. Templates are fairly small and the leak only happens in debug mode, so it hasn't been an issue so far, but we have also discussed switching to Cow<'static, str> in the future. That would be a much more broad breaking change that will require a lot of changes to the core crate and tests. I think it would be better to just continue leaking for now to keep the scope of this PR more manageable.

Currently, the CLI sends a HotReloadedTemplate message to the front end for hot reloading. That is then combined with the dynamic pool to create a full VNode

Right now, the hotreloading is enabled by the following trait (https://github.com/DioxusLabs/dioxus/blob/main/packages/core-types/src/hr_context.rs):

pub trait HotReloadingContext {
    fn map_attribute(
        element_name_rust: &str,
        attribute_name_rust: &str,
    ) -> Option<(&'static str, Option<&'static str>)>;
    fn map_element(element_name_rust: &str) -> Option<(&'static str, Option<&'static str>)>;
}

Meaning that the namespace of the attribute depends on the parent element. If we have to call manganis::register_attribute for each attribute inside each element, the amount of static data generated will be huge.

I have to find a way to compress it, like pointing to an attribute inside the definition of an element

The svg and global attributes traits could be pulled out into a separate "attribute group" section with a name and then each element could include one or more attribute group. For example, a td element would include colspan, and rowspan as special attributes and the string, or hash of GlobalAttribute for the global attribute group

@rambip
Copy link
Author

rambip commented Dec 11, 2024

Ok so something like

  • manganis::register_element(name_of_element, namespace, list_of_attributes, optional_attribute_group)
  • manganis::register_attribute_group(name_of_attribute_group, list_of_attributes)

@rambip
Copy link
Author

rambip commented Dec 12, 2024

/// manganis::register_elemenent!(

I think I will split the macro functionality into 2:

  • manganis::metadata!(name, key)
  • a #[derive(dioxus::ElementSpecification)] as a standard way for the user to define an element.
    I have to define the syntax though. Something like:
#[derive(dioxus::ElementSpecification)]
mod div {
        static NAME_SPACE: &'static str = "div";
        static TAG_NAME: Option<&'static str> = None;
        use x::*;
        static x: Attribute = (...);
}

#[derive(dioxus::AttributeGroupSpecification)]
mod svg_element {
        static x: Attribute = (...);
}

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.

2 participants