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

Implement dynamic tags #1266

Merged
merged 10 commits into from
May 28, 2020
Merged

Implement dynamic tags #1266

merged 10 commits into from
May 28, 2020

Conversation

siku2
Copy link
Member

@siku2 siku2 commented May 26, 2020

Description

Provides an implementation of dynamic tags.

Fixes #1049

Checklist:

  • I have ran ./ci/run_stable_checks.sh
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works -->

Copy link
Member Author

@siku2 siku2 left a comment

Choose a reason for hiding this comment

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

Left a few comments myself to clarify / justify some of my decisions.

yew-macro/src/html_tree/html_dashed_name.rs Show resolved Hide resolved
yew-macro/src/html_tree/html_tag/mod.rs Show resolved Hide resolved
yew-macro/src/html_tree/html_tag/mod.rs Show resolved Hide resolved
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Nice work! Looks like this opens up a few questions about how to handle certain macro checks like value handling and void elements. I don't think they are blockers but I think we should have a plan for them to reduce surprising results when devs use dynamic tags

yew-macro/src/html_tree/html_tag/mod.rs Show resolved Hide resolved
yew-macro/src/html_tree/html_tag/mod.rs Show resolved Hide resolved
yew-macro/src/html_tree/html_tag/mod.rs Outdated Show resolved Hide resolved
yew-macro/src/html_tree/html_tag/mod.rs Outdated Show resolved Hide resolved
yew-macro/src/html_tree/html_tag/mod.rs Show resolved Hide resolved
@siku2
Copy link
Member Author

siku2 commented May 27, 2020

The scope of this PR just got a fair bit broader. I hope you don't mind 😅

@siku2
Copy link
Member Author

siku2 commented May 27, 2020

@jstarry I think at this point it makes sense to add a few tests to check the runtime behaviour of dynamic tags (tags are actually present, void elements, and value attribute handling).
I'm just wondering where I should place these tests?

@jstarry
Copy link
Member

jstarry commented May 27, 2020

@siku2 right now the vtag integration tests are all in src/virtual_dom/vtag.rs, so adding there would be best!

@jstarry
Copy link
Member

jstarry commented May 27, 2020

I hope you don't mind 😅

Don't mind if you don't mind haha. Sometimes it's fun when PR's uncover other things that can be fixed up!

@siku2
Copy link
Member Author

siku2 commented May 27, 2020

Alright, I added some tests and also some runtime checks which make sure that dynamic tags uphold the same invariants as the literal ones.

There are now 2 cases where a dynamic tag will panic:

  1. The generated tag name contains something other than ASCII alphanumerics.
  2. The tag is a void element but has children

This is done essentially the same way as the compile checks, just at runtime. I'm not a huge fan of the hardcoded tag names especially because they're in two places now but so far no better solution has come to mind.


wasm-bindgen-test surprisingly doesn't support should_panic and the panics can't be caught using std::panic::catch_unwind. For this reason I added the two tests for the aforementioned panic cases to the yew-macro crate.

I also noticed that my runtime checks break when the tag names aren't lowercased. So I made sure that the names are converted. But this got me thinking: "what about the literal case?" and sure enough we can break the compile-time checks by doing something like this:

html! {
  <bR>
    <span>{ "haha, void element go brrrrr" }</span>
  </bR>
}

I assumed that non_capitalized_ascii would handle this but it only makes sure that the first letter isn't capitalised (presumably to avoid confusion with Components).
I can't see a legitimate use for non-lowercase tag names so I think we should generate a compile-time error, right?

siku2 added 2 commits May 27, 2020 21:30
By only running it for web-sys. I know, I know.
But seriously, stdweb doesn't seem to have a way to get Element.tagName...
@siku2
Copy link
Member Author

siku2 commented May 27, 2020

I think I just opened another can of worms...
I remembered that even though the HTML standard says tag names only contain alphanumeric characters there's also custom elements which I know must contain a hyphen.
So I took a look and valid custom element names have access to a huge range of characters.

Currently neither literal nor dynamic tags support this entire range.

I 'reverted' f37ebd3#diff-66b9cb4f2a74cf8a59ae2ccb91775f3aR115 for now so dynamic tags support the same characters as literals.
Sadly this means that names like -..div,/ aren't caught early (I don't know if this can cause any issues. It will probably just cause a panic downstream when trying to create the element).

@jstarry
Copy link
Member

jstarry commented May 28, 2020

I can't see a legitimate use for non-lowercase tag names so I think we should generate a compile-time error, right?

Sounds good to me, I doubt this change would bite anyone and it also helps enforce our compile checks in the macro!

So we have a triple standard now:

  1. macro w/ literals
  2. macro w/ dynamic tags
  3. non-macro

Each approach is handled a bit differently. Maybe we could encode this more explicitly by adding a runtime check flag to tag elements?


I think your implementation is great for now, but I do think Yew should strive for reducing surprises in the development process as much as possible. Do you mind writing up an issue for the slight disparities between the 3 approaches I listed above?

@siku2
Copy link
Member Author

siku2 commented May 28, 2020

Maybe we could encode this more explicitly by adding a runtime check flag to tag elements?

That's exactly what I was thinking.

Yew should strive for reducing surprises in the development process as much as possible.

Hard agree on this too 👍

Do you mind writing up an issue for the slight disparities between the 3 approaches I listed above?

Will do! That way I can take a bit of a break and get back to this if someone else hasn't already done it.


I'll just add the lowercase check for the literal case and then I think this PR is just about ready to go.

@siku2
Copy link
Member Author

siku2 commented May 28, 2020

Just realised it can't be a compile error because there are tags like <feGaussianBlur> from the SVG extensions that should be written with that specific capitalisation...
So for now I 'm just using all lowercase where it matters. This means that the original casing is preserved all the way to the VTag.
I think this needs to be addressed when reconciling the differences between the 3 standards so that Yew ignores tag casing internally.

Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Ah, good point. Your code looks great, thanks for the rigor on this!!

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.

Support for dynamic HTML element tags inside html macro
2 participants