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

Fix issues with tuples in closing tag #2886

Merged
merged 5 commits into from Oct 8, 2022

Conversation

hamza1311
Copy link
Member

@hamza1311 hamza1311 commented Sep 25, 2022

Description

This PR:

  • Fix issues with tuples in closing tag
  • Adds test cases for opening tag
  • Cleans up parsing code for components

Fixes #2225

Checklist

  • I have reviewed my own code
  • I have added tests

@hamza1311 hamza1311 added the A-yew-macro Area: The yew-macro crate label Sep 25, 2022
github-actions[bot]
github-actions bot previously approved these changes Sep 25, 2022
@WorldSEnder
Copy link
Member

Can you also add a test that uses a non-self-closing tag? This seems to have had different behaviour in the original bug report, so it would make sense to also test for

    ::yew::html! { <Generic<(u8, bool)> ></Generic<(u8, bool)>> };

@github-actions
Copy link

github-actions bot commented Sep 25, 2022

Visit the preview URL for this PR (updated for commit 3dae63a):

https://yew-rs-api--pr2886-issue-2225-test-azx316a1.web.app

(expires Sat, 15 Oct 2022 15:47:06 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Sep 25, 2022

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 345.572 346.294 345.866 0.261
Hello World 10 610.369 615.386 611.275 1.515
Function Router 10 2238.803 2249.063 2243.240 3.655
Concurrent Task 10 1007.582 1009.669 1008.799 0.632

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 307.137 308.251 307.705 0.403
Hello World 10 623.885 630.246 625.876 2.289
Function Router 10 2265.820 2281.488 2275.193 5.300
Concurrent Task 10 1007.629 1009.731 1008.725 0.695

@github-actions
Copy link

github-actions bot commented Sep 25, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 106.234 106.237 +0.003 +0.003%
boids 170.388 170.381 -0.007 -0.004%
communication_child_to_parent 90.108 90.104 -0.004 -0.004%
communication_grandchild_with_grandparent 104.903 104.904 +0.001 +0.001%
communication_grandparent_to_grandchild 100.771 100.762 -0.009 -0.009%
communication_parent_to_child 87.213 87.213 0 0.000%
contexts 107.390 107.386 -0.004 -0.004%
counter 85.131 85.128 -0.003 -0.003%
counter_functional 85.603 85.600 -0.003 -0.003%
dyn_create_destroy_apps 87.965 87.965 0 0.000%
file_upload 99.701 99.699 -0.002 -0.002%
function_memory_game 162.850 162.853 +0.003 +0.002%
function_router 346.953 346.953 0 0.000%
function_todomvc 157.652 157.653 +0.001 +0.001%
futures 222.000 222.000 0 0.000%
game_of_life 104.649 104.650 +0.001 +0.001%
immutable 181.197 181.189 -0.008 -0.004%
inner_html 81.992 81.992 0 0.000%
js_callback 110.930 110.917 -0.013 -0.011%
keyed_list 195.484 195.485 +0.001 +0.000%
mount_point 84.738 84.739 +0.001 +0.001%
nested_list 111.660 111.660 0 0.000%
node_refs 92.611 92.611 0 0.000%
password_strength 1546.768 1546.764 -0.004 -0.000%
portals 95.975 95.971 -0.004 -0.004%
router 316.771 316.758 -0.013 -0.004%
simple_ssr 151.691 151.692 +0.001 +0.001%
ssr_router 392.098 392.102 +0.004 +0.001%
suspense 108.741 108.736 -0.005 -0.004%
timer 88.019 88.017 -0.002 -0.002%
todomvc 138.956 138.958 +0.002 +0.001%
two_apps 85.792 85.793 +0.001 +0.001%
web_worker_fib 152.043 152.042 -0.001 -0.001%
webgl 84.457 84.460 +0.003 +0.003%

✅ None of the examples has changed their size significantly.

@hamza1311
Copy link
Member Author

Can you also add a test that uses a non-self-closing tag? This seems to have had different behaviour in the original bug report, so it would make sense to also test for

    ::yew::html! { <Generic<(u8, bool)> ></Generic<(u8, bool)>> };

Oh right, that's what the original bug report was about then. I can confirm that only non-self closing tags panic!

@hamza1311 hamza1311 marked this pull request as draft September 25, 2022 16:01
@hamza1311 hamza1311 changed the title Add test case for tuple generics in html! Fix issues with tuples in closing tag Sep 25, 2022
@hamza1311 hamza1311 marked this pull request as ready for review September 25, 2022 17:03
siku2
siku2 previously approved these changes Sep 30, 2022
packages/yew-macro/src/html_tree/html_component.rs Outdated Show resolved Hide resolved

error: this closing tag has no corresponding opening tag
--> $DIR/generic-component-fail.rs:45:30
| ^^^^^^^ expected 1 generic argument
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Removing hand-written parsing and getting better errors!

github-actions[bot]
github-actions bot previously approved these changes Oct 3, 2022
github-actions[bot]
github-actions bot previously approved these changes Oct 3, 2022
Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

Some further small details now that the end type is actually checked.

Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

Looks good to me now!

@hamza1311 hamza1311 merged commit 7ada344 into yewstack:master Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-macro Area: The yew-macro crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic tuple type panicks html macro in the closing tag
3 participants