seed icon indicating copy to clipboard operation
seed copied to clipboard

Typed attributes - API implementation

Open MartinKavik opened this issue 4 years ago • 12 comments

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 ?

Originally posted by @MuhannadAlrusayni in https://github.com/seed-rs/seed/issues/312#issuecomment-590794672

MartinKavik avatar Feb 25 '20 12:02 MartinKavik

I would prefer the following. It seems more "rustic" to use constructor methods

input![
    At::title("Enter your name"),
    At::required(true)
]

Also have a look at https://github.com/nrc/derive-new and https://github.com/colin-kiegel/rust-derive-builder

nielsle avatar Feb 25 '20 12:02 nielsle

@nielsle Please read https://github.com/seed-rs/seed/issues/312 and continue there with reasons why it's better than the original proposal, thanks!

MartinKavik avatar Feb 25 '20 12:02 MartinKavik

I would prefer At::title("..") too.

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

Foo.do_somehting()

and I had hard time to figure out how this code worked, since Foo 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 = Foo;
x.do_something()

So I think At.do_something() is not beginner friendly.

Moved

MuhannadAlrusayni avatar Feb 25 '20 12:02 MuhannadAlrusayni

@MuhannadAlrusayni Please use the previous (referenced above) issue for design and this one for implementation.

MartinKavik avatar Feb 25 '20 12:02 MartinKavik

if you guys have any suggestion

Try to make it as simple as possible.. I didn't think about it so much to write some reasonable advice and we'll see during implementation how it goes - maybe there will be multiple iterations before we make it good enough.

MartinKavik avatar Feb 26 '20 14:02 MartinKavik

A problem with this implementation is that the constructor method have to pick an initial value for the attribute, like what we have did for Dir attribute, we picked Dir::Auto which will default to the browser default, but not all HTML attributes have value that default to the browser default (e.g. rel, scope, ..etc).

There are a few solutions I can think of:

  1. constructor method take the initial value, something like A.rel(Rel::Author)
    • Cons:
      1. users have to write the attribute name twice
      2. users have to import Rel enum to scope so they can use it's variants.
  2. we convert all attributes values into stand alone struct and we use these stand alone struct in enum and that enum would be the value for the attribute
    • Cons
      1. There will be a lot of structs
      2. Users have to import val module to scope so they can use these stand alone structs
    • Pros
      1. This solve the cons a in the 1st solution.
  3. we provide constructor method for each attribute value, something like A.rel_author()
    • Cons
      1. users cannot pass stored attribute value to attribute constructor method:
        struct Model {
            dir: Dir,
        }
        // view function
        fn view_model(&self) -> Node<Msg> {
            p![
                match self.dir {
                    Dir::RTL => A.dir_rtl(),
                    Dir::LTR => A.dir_ltr(),
                    Dir::Auto => A.dir_auto(),
                },
            ]
        }
        

Implementation

Implementation for the 2nd solution:

pub mod val {
    #[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, Display)]
    #[display(fmt = "alternate")]
    pub struct Alternate;

    #[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, Display)]
    #[display(fmt = "author")]
    pub struct Author;

    #[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, Display)]
    #[display(fmt = "dns-prefetch")]
    pub struct DnsPrefetch;

    #[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, Display)]
    #[display(fmt = "hrlp")]
    pub struct Help;

    #[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, Display)]
    #[display(fmt = "icon")]
    pub struct Icon;

    // ... the rest of the attribute value structs
}

#[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Copy, Clone, From, Display)]
pub enum Rel {
    #[from]
    Alternate(val::Alternate),
    #[from]
    Author(val::Author),
    #[from]
    DnsPrefetch(val::DnsPrefetch),
    #[from]
    Help(val::Help),
    #[from]
    Icon(val::Icon),
    // ... the rest of the attribute value variant
}

impl A {
    pub fn rel(&self, value: impl Into<Rel>) -> Rel { value.into() }
}

Usage

This is what users would write with: 1st solution:

use seed::attr::{A, Dir, Types};

input![
    A.dir(Dir::RTL),
    // since `type` is keyword we cannot use it.. we can figure out what's the best way to name this method later.
    A.types(Types::Number), // we can pass string too A.types("text/css") since `type` can be string too.
    A.required(true),
    A.step(0.2),
]

2ed solution:

use seed::attr::{A, val};

input![
    A.dir(val::RTL),
    // since `type` is keyword we cannot use it.. we can figure out what's the best way to name this method later.
    A.types(val::Number), // we can pass string too A.types("text/css") since `type` can be string too.
    A.required(true),
    A.step(0.2),
]

3nd solution:

use seed::attr::A;
input![
    A.dir_rtl(),
    // note that we don't need to use the method name `type()` with this solution 
    A.type_number(),
    // we can have A.type_from("text/css") since `type` attribute can be string too.
    A.required(true),
    A.step(0.2),
]

I would prefer solution number 2, what do you guys prefer and why ? or do you have better solution for this problem ?

MuhannadAlrusayni avatar Feb 26 '20 15:02 MuhannadAlrusayni

what do you guys prefer and why ?

I'd prefer 1) because it's explicit. Using val like in the second solution might lead to name conflicts.

flosse avatar Feb 26 '20 15:02 flosse

Using val like in the second solution might lead to name conflicts.

+1, I agree ~~, but I scanned all HTML attributes and I didn't find any conflicts yet.~~ found some :D

MuhannadAlrusayni avatar Feb 26 '20 16:02 MuhannadAlrusayni

4th solution (as designed - https://github.com/seed-rs/seed/issues/312#issuecomment-565832751):

input![
    A.dir().rtl(),
    A.type_().number(),
    A.type_().content_type().text_css(),
    A.required(true),
    A.step(0.2),
]
  • Doesn't pollute use and local scope.
  • Autocomplete friendly.
  • Respects conventions for keywords in the name (e.g. type_).
  • Uses native types (e.g. bool and f64) as arguments to minimize verbosity.
  • As typed, simple and consistent as possible.

@MuhannadAlrusayni

  • Why are you changing design?
  • Why are you changing design in the issue for implementation?

MartinKavik avatar Feb 26 '20 18:02 MartinKavik

  • Why are you changing design?
  • Why are you changing design in the issue for implementation?

I didn't change design .. I had an issue with the current implementation (I mentioned above https://github.com/seed-rs/seed/issues/371#issuecomment-591479686) and I was suggestion some solutions that can fit with the current implementation.

Your solution doesn't solve the issue I mentioned above. To make the issue clean, what will happen when users do this (using current implementation or your 4th solution too) ?:

input![
    // what's the result ? what's the attribute value we get here ?
    A.dir(),
    // what about this attribute
    A.type_(),
]

MuhannadAlrusayni avatar Feb 26 '20 18:02 MuhannadAlrusayni

input![
    // what's the result ? what's the attribute value we get here ?
    A.dir(),
    // what about this attribute
    A.type_(),
]

Both return a specific struct - e.g. attr::Dir / attr::Type. The important is that they don't implement UpdateEl so the compilation fails and it force you to pick the value.

MartinKavik avatar Feb 26 '20 19:02 MartinKavik

Both return a specific struct - e.g. attr::Dir / attr::Type. The important is that they don't implement UpdateEl so the compilation fails and it force you to pick the value.

Well, they do implement UpdateEl, as I said in the implementation steps in 1st comment:

3- implement UpdateEl<..> for all attributes types we have created

Even if they didn't implement UpdateEl what's the value returned from e.g. A.type_() ? and how we plan to add the returned value to el.attrs without implementing UpdateEl ?

MuhannadAlrusayni avatar Feb 26 '20 20:02 MuhannadAlrusayni