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

Allow boolean & update error for byte literals in html! #3441

Merged
merged 4 commits into from
Oct 12, 2024

Conversation

its-the-shrimp
Copy link
Contributor

Description

Adds support for the following usage of the html! macro:

html!{<>
  { b'a' }
  { true }
</>}
  • I have reviewed my own code
  • I have added tests

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Visit the preview URL for this PR (updated for commit 558b30c):

https://yew-rs-api--pr3441-allow-more-literals-ppr0g8os.web.app

(expires Sat, 04 Nov 2023 18:38:54 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 103.070 103.070 0 0.000%
boids 176.629 176.629 0 0.000%
communication_child_to_parent 95.648 95.648 0 0.000%
communication_grandchild_with_grandparent 108.768 108.768 0 0.000%
communication_grandparent_to_grandchild 105.178 105.178 0 0.000%
communication_parent_to_child 93.126 93.126 0 0.000%
contexts 112.460 112.460 0 0.000%
counter 89.818 89.818 0 0.000%
counter_functional 90.479 90.479 0 0.000%
dyn_create_destroy_apps 93.040 93.040 0 0.000%
file_upload 104.083 104.083 0 0.000%
function_memory_game 175.535 175.535 0 0.000%
function_router 351.553 351.553 0 0.000%
function_todomvc 164.044 164.044 0 0.000%
futures 232.037 232.037 0 0.000%
game_of_life 112.904 112.904 0 0.000%
immutable 190.158 190.158 0 0.000%
inner_html 86.141 86.141 0 0.000%
js_callback 113.525 113.525 0 0.000%
keyed_list 202.478 202.478 0 0.000%
mount_point 89.028 89.028 0 0.000%
nested_list 117.085 117.085 0 0.000%
node_refs 96.723 96.723 0 0.000%
password_strength 1753.727 1753.727 0 0.000%
portals 97.796 97.796 0 0.000%
router 320.700 320.700 0 0.000%
simple_ssr 143.755 143.755 0 0.000%
ssr_router 389.493 389.493 0 0.000%
suspense 119.868 119.868 0 0.000%
timer 92.348 92.348 0 0.000%
timer_functional 101.132 101.132 0 0.000%
todomvc 144.134 144.134 0 0.000%
two_apps 89.880 89.880 0 0.000%
web_worker_fib 138.090 138.090 0 0.000%
web_worker_prime 188.784 188.784 0 0.000%
webgl 88.748 88.748 0 0.000%

✅ None of the examples has changed their size significantly.

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 428.970 457.062 440.000 9.633
Hello World 10 704.175 731.213 714.540 8.832
Function Router 10 2377.232 2459.566 2409.243 22.991
Concurrent Task 10 1008.319 1011.532 1010.472 0.918
Many Providers 10 1627.578 1698.256 1653.789 23.599

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 424.808 460.220 438.605 10.296
Hello World 10 704.032 738.847 719.232 12.083
Function Router 10 2393.186 2483.879 2442.656 32.208
Concurrent Task 10 1008.987 1011.855 1010.393 0.910
Many Providers 10 1636.526 1687.289 1667.949 16.303

ranile
ranile previously approved these changes Oct 4, 2023
Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Since the VDOM API supports it, I don't see why the macro shouldn't. Thanks for the PR!

github-actions[bot]
github-actions bot previously approved these changes Oct 4, 2023
@ranile ranile dismissed github-actions[bot]’s stale review October 4, 2023 17:59

GH actions should not have approved this

Copy link
Member

@cecton cecton left a comment

Choose a reason for hiding this comment

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

I'm not sure why/how a byte would be useful? A byte is literally a u8, so it's converting b'a' to the string "97"?

@its-the-shrimp
Copy link
Contributor Author

It would indeed be strange to use a byte literal in HTML, but who knows what use cases are there in the wild, I think it would be even more strange to pretend like it's not stringifiable at compile-time.

@ranile
Copy link
Member

ranile commented Oct 5, 2023

That is a valid point. What if we improved the diagnostic instead of just saying "invalid type"?

cecton
cecton previously approved these changes Oct 5, 2023
@its-the-shrimp
Copy link
Contributor Author

What if we improved the diagnostic instead of just saying "invalid type"?

I did just that for byte-string literals in this PR, the only Rust literal kind that generates a non-Displayable value.

packages/yew-macro/src/stringify.rs Outdated Show resolved Hide resolved
packages/yew-macro/src/html_tree/html_node.rs Outdated Show resolved Hide resolved
packages/yew-macro/src/stringify.rs Outdated Show resolved Hide resolved
packages/yew-macro/src/stringify.rs Show resolved Hide resolved
Lit::Float(v) => v.base10_digits().into(),
Lit::Bool(v) => if v.value() { "true" } else { "false" }.into(),
Lit::ByteStr(v) => String::from_utf8(v.value()).ok()?.into(),
Lit::Byte(v) => v.value().to_string().into(),
Copy link
Member

Choose a reason for hiding this comment

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

This is similar to ByteStr, however since a byte is u8 and u8 supports Display.
So I am not sure not allowing b'A' would be effective.

@its-the-shrimp its-the-shrimp dismissed stale reviews from cecton and ranile via 11c1824 October 28, 2023 12:18
Copy link
Member

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@ranile ranile changed the title Allow boolean & byte literals in html! Allow boolean & update error for byte literals in html! Oct 12, 2024
@ranile ranile merged commit 4978b99 into yewstack:master Oct 12, 2024
21 checks passed
@ranile
Copy link
Member

ranile commented Oct 12, 2024

I'm sorry it took me this long to merge 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.

4 participants