Seed: Typed Attributes - API design

[MartinKavik]

See https://github.com/seed-rs/seed/pull/311#issuecomment-565406617.

[MartinKavik]

Another draft:

div![
    vec![ // vs `attrs!` or `Attrs::from` or nothing (`div![ A.disabled ]`) ..?  (A)  
        A.class("foo"),

        if disabled { A.disabled } else { None },
        // A.disabled.filter(|| disabled)
        // disabled.then(A.disabled).flatten()  (nightly)
        // disabled.flat_then(A.disabled)       (with extra trait for bool)
        // .. ? (B)

        A.autocomplete.off,
        A.autofocus,

        Attr::new_with_value("data-columns", "3"),   // = custom attribute
        // A::new_with_value( ..
        // A::custom("data-columns", Some("3")),
        // A.custom( ..
        // .. ? (C)
    ]
]

Rust playground (_note:_ &'static str should be refactored to something more flexible like Into<Cow.. or impl ToString ; Attr's functions probably shouldn't return Option<Attrs> ; etc.)

Reasoning:

• Readability - A.class should be a little bit more readable than A::class because the first name letter is higher than the previous char (.). And we don't need extra () at the end because of . notation (structs + fields vs modules + functions). A::Class cannot be used because we can't chain them and their parameters aren't as flexible as in functions.

  • Writeability - A.autocomplete.off - you have to write only essential identifiers and dividers and it's IDE autocomplete friendly.

  • Explicitness - it's clear that A.disabled can't have any value, A.autocomplete can have only predefined value and A.class can have only once custom value.

  • Typed - it should be impossible to write attributes which doesn't make sense according to HTML spec (custom attribute is exception). However it will still be possible to add the attribute to an incompatible element and we won't be validating custom values.

What are your opinions about the draft?
And what about points A, B, C (see example code)?

(cc @TatriX)

[TatriX]

Looks interesting. How does this approach affect binary size? Can we somehow do the same but with ZST?

[MartinKavik]

What is ZST?

The same approach is implemented in webpack quickstart (generated css_classes.rs has cca 83k lines) - it looks like the compiler removes unused properties.
_Note:_ It's Important to use static for the struct with values (instead of const) otherwise it fails in runtime with out of memory error (or something like that I don't remember the exact error).

[TatriX]

ZST

println!("Size: {:?}", std::mem::size_of::<Attributes>()); // 128
println!("Size: {:?}", std::mem::size_of::<[Attributes; 5]>()); // 640

[David-OConnor]

I like it.

[TatriX]

Another option (playground)

fn main() {
    let disabled = true;

    let rendered = div(vec![
        attr::class("foo"),
        attr::disabled(disabled),
        attr::autocomplete::off(),
        attr::autofocus(),
        attr::custom("data-columns", "3"),
    ]);

    let expected =
        r#"<div class="foo" disabled autocomplete="off" autofocus data-columns="3"></div>"#;

    assert_eq!(rendered, expected);

    {
        use attr::*;

        let rendered = div(vec![
            class("foo"),
            disabled(false),
            autocomplete::on(),
            data("columns", "3"),
        ]);
        let expected = r#"<div class="foo" autocomplete="on" data-columns="3"></div>"#;

        assert_eq!(rendered, expected);
    }
}

[TatriX]

This one is based on Martin's version with functions instead of fields:

    let rendered = div(&[
        A.class("foo"),
        A.disabled(disabled),
        A.autocomplete().on(),
        A.autofocus(),
        A.custom("data-columns", "3"),
    ]);

playground

I guess main advantage is that all available attributes can are all the methods on Attr struct, compared to fields/methods of the original.

BTW, does auto-completion work for you with static variables? I need to introduce a local variable with type hint to make it work for Emacs/rls.

[MartinKavik]

let rendered = div(vec![
    attr::class("foo"),
    attr::disabled(disabled),
    attr::autocomplete::off(),
    attr::autofocus(),
    attr::custom("data-columns", "3"),
]);

• attr::autofocus() - missing bool parameter ("typo"?)
• attr::custom("data-columns", "3") - how can I create a custom boolean attribute (i.e. attr without a value)?
• I don't like parameter in boolean attributes very much because:
  
  
  
  • It deviates from HTML spec and especially it deviates from the real representation in DOM - I need to think a little bit where I'm comparing my code and DOM in browser's inspector, because I can't compare it only visually (I see attribute in code / I see attribute in DOM vs None / I don't see attribute in DOM).
  
  
  • It's unnecessary if we don't need to switch its value. It makes code longer, you have to create a new variable and there is a possibility to write a bug (you accidentally write false instead of true).
  
  
• attr::autocomplete::off() vs A.autocomplete.off - I think it's harder to read / write.

---

use attr::*;
let rendered = div(vec![
    class("foo"),
    disabled(false),
    autocomplete::on(),
    data("columns", "3"),
]);

• I think there would be conflicts with your local variables (class, data, disabled,..). And something like disabled(disabled) doesn't look very well imho.
• Too implicit - you can't recognize that they are attributes. Longer div with more entities would be unreadable.

---

I guess main advantage is that all available attributes can are all the methods on Attr struct, compared to fields/methods of the original.

What's the advantage from the users point of view?

BTW, does auto-completion work for you with static variables? I need to introduce a local variable with type hint to make it work for Emacs/rls.

If I remember correctly it works in IntelliJ IDEA - but only if the struct isn't too big and when you don't write it inside a macro call.. so.. actually it doesn't work. I hope that https://github.com/rust-analyzer/rust-analyzer will resolve our problems in the future.

---

ZST - I'm not familiar with it, but it looks good. If it helps with the public API or the implementation and works with stable, we should use it.

[TatriX]

My gut feeling tells me that in real code you mostly use disabled with some state controlling it. As such writing if disabled { A.disabled } else { None }, is more annoying than something like A.disabled(state.disabled). I think in js it's still something like

$("id").disabled = true
// or
$("id").setAttribute("disabled", "true");

What's the advantage from the users point of view?

That you can open docs and see a homogeneous list of all available attributes.

use attr::*;

It was just to show that it's possible to use it this way.

I don't really have strong opinion on readability/writeability here. The only thing I know that currently A.foo doesn't work very well with autocompletion. But I think you are right, that A.foo.bar is a bit easier to read and write.

Probably we should continue with a comprehensive list of tests, to check if proposed solution is actually useful.

[MartinKavik]

My gut feeling tells me that in real code you mostly use disabled with some state controlling it.

Ok, I like facts more than gut feeling, but when you look at the list of boolean attributes, it looks like the most used/"famous" ones on the list are disabled, checked, readonly, selected and some other. So, yeah, I think that you are right, you want to control them in the most cases. And if you are coming from the Elm world you are already familiar with this syntax - Html.Attributes.

That you can open docs and see a homogeneous list of all available attributes.

Valid point. But I think you will be looking mostly to your IDE (once the autocomlete works properly) and it doesn't really matter here.
I tried to add function as the field value to make them consistent, but as the result you would use it like (A.disable)(true).

Probably we should continue with a comprehensive list of tests, to check if proposed solution is actually useful.

I agree.

---

Updated proposal:

div![
    A.class("foo"),
    A.disabled(disabled),
    A.autocomplete().on(),
    A.autofocus(true),
    A.custom("data-columns", "3"),
    A.custom("custom-boolean", None),
]
// or with wrapper
div![vec![   // or &[ or Iterator ...
    A.class("foo"),
    // ...
]]

• All items are functions. Benefits:
  
  
  
  • The documentation is consistent.
  
  
  • You don't have to think if write ( at the end or not.
  
  
  • A.autocomplete().on() is maybe a little bit more readable than A.autocomplete.on because () works as the divider between the name and the value.
  
  
• A.custom("name", value) - value has to implement a predefined trait. That trait should be implemented at least for &str and Option<&str>. (Or for another more flexible &str alternative.)
• Attributes can be placed directly into the element macro call:

a![ A.href("http://example.com") ]

[TatriX]

Looks good to me!

[MartinKavik]

I don't know how to make it better now. We will see how it works during implementation and testing with the real projects.
@David-OConnor Do you see any problems with the design?

@TatriX (cc @David-OConnor) I won't be able to contribute to Seed / respond to comments for the next 2-3 weeks. But if you want to investigate it more or start with implementation I can write you some required steps.

[TatriX]

Sure, do me a favor!

[MartinKavik]

Guide for implementation

Let me describe how typed styles are integrated - attributes, tags and events work almost the same way, but styles integration is the only completed one. You can use it for the inspiration and it should help you to navigate through the code base.

1. generator.rs creates on push or every Saturday new css_properties.json.
2. When you run cargo make populate_all in Seed repo, style_names.rs is regenerated.
3. style_names.rs uses macro make_styles from styles.rs and creates enum St with typed styles.
4. When you write div![ style!{ St::Width => px(5) } ]:
   
   
   
   1. Macro style is invoked and creates a new Style instance.
   
   
   2. Style implements UpdateEl, so element macros can call update and then element's styles are finally updated.
   
   

Notes:

• I'll add you as the collaborator to html-css-db with write permissions. You can copy generator.rs or you can refactor it to proper Rust app - your choice. You don't need to wait for code reviews in this repo, I'll look at it once I'm back.
• Don't be afraid to change directory structure and file names in Seed if it makes sense to you. Attributes will be used as the inspiration for future refactors of the other entities.
• /examples can be updated later, but at least some unit tests should be written in virtual_dom.rs.
• Try to keep the the old attributes API usable so we don't need to make a big breaking change. But it shouldn't be at the expense of the new API. Mark the old API + implementation as deprecated (probably since 0.6.0).
• Write (public API) changes into the changelog.

Thanks and good luck!

[TatriX]

Unfortunately I won't be able to work on this in Jan because work & vacation. So please don't wait for me if any.

[MartinKavik]

Neither do I, because I focus on VDOM and style lib.
Assign it to yourself, please, once you have time for this. And enjoy your vacation 🍹

Or anybody else could try to do this.

[MuhannadAlrusayni]

Have anyone worked on this ? what is the state ? can I have it ?

[MartinKavik]

I think you can take it. cc @TatriX

[TatriX]

Yes, you can. I'm working on the Fetch API right now. TBH, I get used to the current attrs implementation ;)

[MartinKavik]

I think there should be also A.none as the equivalent to empty![]. And we should flag id! as deprecated - A.id is better alternative.

[MuhannadAlrusayni]

Updated proposal:

div![
    A.class("foo"),
    A.disabled(disabled),
    A.autocomplete().on(),
    A.autofocus(true),
    A.custom("data-columns", "3"),
    A.custom("custom-boolean", None),
]
// or with wrapper
div![vec![   // or &[ or Iterator ...
    A.class("foo"),
    // ...
]]

I start to work on this, and I would like to share the way I follow so if you guys have any suggestion:
1- Create unit struct A (pub struct A;)
2- Create custom type for each HTML attribute
3- implement UpdateEl<..> for all attributes types we have created
4- Add constructor method for all attributes types in impl A { /* constructor methods */ }

examples:

pub struct A;

impl A {
    // constructor method for title attribute
    pub fn title(&self, value: impl Into<Title>) -> Title {
        value.into()
    }

    // constructor method for checked attribute
    pub fn required(&self, value: impl Into<Required>) -> Required {
        value.into()
    }

    // constructor method for title attribute
    pub fn dir(&self) -> Dir {
        Dir::Auto
    }
}

#[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, From, Display)]
pub enum Dir {
    #[display(fmt = "ltr")]
    LTR,
    #[display(fmt = "rtl")]
    RTL,
    #[display(fmt = "auto")]
    Auto,
}

impl Dir {
    pub fn ltr(self) -> Dir { Dir::LTR }
    pub fn rtl(self) -> Dir { Dir::RTL }
    pub fn auto(self) Dir -> { Dir::Auto }
}

impl<Ms> UpdateEl<El<Ms>> for Dir {
    fn update(self, el: &mut El<Ms>) {
        el.attrs.add(At::Dir, self);
    }
}

#[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, From)]
pub struct Required(bool);

impl<Ms> UpdateEl<El<Ms>> for Required {
    fn update(self, el: &mut El<Ms>) {
        el.attrs.add(At::Required, self.0.as_at_value());
    }
}

#[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, From, Display)]
pub struct Title(Cow<'static, str>);

impl<Ms> UpdateEl<El<Ms>> for Title {
    fn update(self, el: &mut El<Ms>) {
        el.attrs.add(At::Title, self.0);
    }
}

with this we can write:

input![
    A.title("Enter your name"),
    A.required(true),
    A.dir().rtl(),
]

are there any improvement or suggestion till now ?

[MuhannadAlrusayni]

comment on the current proposal, I would like to use At::function() instead of At.method()

I remember when I saw a library that uses this way:

At.method()

and I had hard time to figure out how this code worked, since At is a type and I didn't know that unit struct can be used as instance of their types, so the previous snippet is shortcut to:

let x = At;
x.method()

So I think At.method() is not beginner friendly, and At::function() is widely used in Rust community.

[MartinKavik]

@MuhannadAlrusayni
At::function() - what would be equivalent to A.autocomplete().on() in general form e.g. A.foo(x).bar(y)?

[MartinKavik]

@MuhannadAlrusayni Sorry, I deleted duplicated comment and you probably done the same.. And GitHub has some problems today.

Original comment:

---

@MartinKavik

• A.autocomplete().on() would be At::autocomplete.on().
• A.foo(x).bar(y) would be At::foo(x).bar(y).

we can also use At::autocomplete::on() but I wouldn't prefer that.

---

[MartinKavik]

A::foo(x).bar(y) vs B.foo(x).bar(y)

• B.foo is more readable/scannable, especially with the method with the first small char: A::src vs B.src.
• While writing B.foo().bar().x() you don't have to think if you should use :: or ..
• It's proved in Quickstart with Webpack for writing classes - class![C.main, C.p_4].
• I don't think it will be a problem for beginners - they don't know Rust very well and many other languages use only dots.

---

At::autocomplete::on() - it has the same disadvantages like A::foo().bar() but it's also much less flexible.

[MuhannadAlrusayni]

I don't really mind to use B.foo() , I just want to point that it's not widely used in Rust and is not beginner friendly (in my opinion) for the reason mentioned above.

[MuhannadAlrusayni]

Alternative proposal

usage would be, something like this:

use seed::attr::{a, id, class, value, Type, required, checked, custom};
input![
    a(id("login")),
    a(class("bla")),
    // we can pass slice to class, we have more control on arguments types since it's a function.
    a(class(&["bla1", "bla2"])),
    a(value("bla"),
    // we can use types directly instead of calling functions
    a(Type::Checkbox),
    a(required(true)),
    a(checked(true)),
    a(custom("name", "value")),
]

this proposal solve most of the problems found in https://github.com/seed-rs/seed/issues/371#issuecomment-591479686 and I believe it's easier to use and easier to implement.

[MartinKavik]

• It pollutes local scope (id, class, value, checked, etc. are very common variable names).
• It pollutes imports (use).
• We need macro for class because functions aren't flexible enough - e.g. conditionals are verbose in functions (Something like a(class(&[("active", active).into(), "btn".into()])) vs class!["active" => active, "btn"]).
• a(.. isn't very autocomplete friendly.
• a can cause conflicts with local variables.

• a( is relatively easily interchangeable with a![ during scanning.

• _"easier to implement"_ - it has very low priority.

[MuhannadAlrusayni]

• It pollutes local scope (id, class, value, checked, etc. are very common variable names).
• It pollutes imports (use).
• a can cause conflicts with local variables.
• a( is relatively easily interchangeable with a![ during scanning.

We can use the module name to call constructor functions, and I didn't choose a it is used in the current proposal although it was A, personally I would prefer something longer, 3 characters maybe, something like this:

fn view() -> Node<Msg> {
    use seed::attr::{att, id, class, value, Type, required, checked, custom};
    // we can do this too, since we are importing them to the view function only
    // use seed::attr::*;
    // or
    // use seed::attr::{att, self};

    input![
        att(id("login")),
        att(class("bla")),
        // we can pass slice to class, we have more control on arguments types since it's a function.
        att(class(&["bla1", "bla2"])),
        att(value("bla"),
        // we can use types directly instead of calling functions
        att(Type::Checkbox),
        att(required(true)),
        att(checked(true)),
        att(custom("name", "value")),
    ]
}

• We need macro for class because functions aren't flexible enough - e.g. conditionals are verbose in functions (Something like a(class(&[("active", active).into(), "btn".into()])) vs class!["active" => active, "btn"]).

we can use class!... and we can achieve the same with class() function if we want to.

• a(.. isn't very autocomplete friendly.

The current implementation isn't autocomplete friendly if we use some sort of macro to do the work for us which is eventuality what we will do.

[MartinKavik]

Closing in favor of https://github.com/seed-rs/seed/issues/525.