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.

Sign up for GitHub

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

Jump to bottom

An ever Increasing Component ID #2537

Merged
merged 9 commits into from Mar 21, 2022
Merged

An ever Increasing Component ID #2537

merged 9 commits into from Mar 21, 2022

Conversation

futursolo
Copy link
Member

Description

Originally part of #2453.

This pull request makes the parent's component ID to always be smaller than child components.

The effect of this change:

  1. The parent will always render before children.
  2. The rendered lifecycle of child components will always be called before parent.

These changes are done to facilitate SSR hydration so that NodeRef of parent elements can always be fixed before child elements.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@futursolo
Bring changes to this branch.
ecd2f6a
@futursolo
Child components always render after parents.
fc8a323
@futursolo
Merge branch 'master' into comp-id
5d40942 
# Conflicts:
#	.github/workflows/main-checks.yml
#	examples/agents/Cargo.toml
#	examples/boids/Cargo.toml
#	examples/contexts/Cargo.toml
#	examples/counter/Cargo.toml
#	examples/dyn_create_destroy_apps/Cargo.toml
#	examples/file_upload/Cargo.toml
#	examples/function_memory_game/Cargo.toml
#	examples/function_router/Cargo.toml
#	examples/function_router/index.html
#	examples/function_todomvc/Cargo.toml
#	examples/futures/Cargo.toml
#	examples/game_of_life/Cargo.toml
#	examples/inner_html/Cargo.toml
#	examples/js_callback/Cargo.toml
#	examples/keyed_list/Cargo.toml
#	examples/mount_point/Cargo.toml
#	examples/nested_list/Cargo.toml
#	examples/node_refs/Cargo.toml
#	examples/password_strength/Cargo.toml
#	examples/portals/Cargo.toml
#	examples/router/Cargo.toml
#	examples/suspense/Cargo.toml
#	examples/timer/Cargo.toml
#	examples/todomvc/Cargo.toml
#	examples/two_apps/Cargo.toml
#	examples/web_worker_fib/Cargo.toml
#	examples/webgl/Cargo.toml
#	packages/yew-router/Cargo.toml
#	packages/yew/Cargo.toml
#	packages/yew/Makefile.toml
#	packages/yew/src/app_handle.rs
#	packages/yew/src/dom_bundle/bcomp.rs
#	packages/yew/src/dom_bundle/mod.rs
#	packages/yew/src/dom_bundle/traits.rs
#	packages/yew/src/html/component/lifecycle.rs
#	packages/yew/src/html/component/mod.rs
#	packages/yew/src/html/component/scope.rs
#	packages/yew/src/html/mod.rs
#	packages/yew/src/lib.rs
#	packages/yew/src/renderer.rs
#	packages/yew/src/scheduler.rs
#	packages/yew/src/suspense/component.rs
#	packages/yew/src/suspense/mod.rs
#	packages/yew/src/virtual_dom/vcomp.rs
#	tools/website-test/Cargo.toml
#	website/docs/getting-started/build-a-sample-app.mdx
#	website/docs/tutorial/index.mdx
@futursolo
Cleanup residual Portal references.
b605a9d
@futursolo
Fix after merge.
8d57d05
@futursolo
Fix trybuild.
48ea85d
github-actions[bot]
github-actions bot previously approved these changes Mar 20, 2022
View reviewed changes
@github-actions
Copy link

github-actions bot commented Mar 20, 2022 •
edited

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

https://yew-rs-api--pr2537-comp-id-u71lgz64.web.app

(expires Mon, 28 Mar 2022 04:06:45 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Mar 20, 2022 •
edited

Size Comparison

examples master (KB) pull request (KB) diff
boids 304.595 309.091 +4.496
contexts 226.549 230.852 +4.303
counter 157.687 162.898 +5.212
dyn_create_destroy_apps 166.176 170.953 +4.777
file_upload 187.256 193.270 +6.014
function_memory_game 344.588 349.035 +4.447
function_router 22.254 22.254 0
function_todomvc 320.395 324.554 +4.159
futures 356.007 361.689 +5.683
game_of_life 199.937 205.580 +5.644
inner_html 149.620 154.724 +5.104
js_callback 165.002 170.220 +5.218
keyed_list 322.361 327.117 +4.756
mount_point 156.794 162.055 +5.261
nested_list 218.416 222.912 +4.496
node_refs 164.282 169.042 +4.760
password_strength 1845.914 1849.870 +3.956
portals 170.833 175.582 +4.749
router 582.700 584.875 +2.175
suspense 215.878 220.498 +4.620
timer 163.490 168.722 +5.231
todomvc 263.836 268.902 +5.066
two_apps 159.090 164.346 +5.256
webgl 163.839 169.069 +5.230

github-actions[bot]
github-actions bot previously approved these changes Mar 20, 2022
View reviewed changes
github-actions[bot]
github-actions bot previously approved these changes Mar 20, 2022
View reviewed changes
@WorldSEnder WorldSEnder added performance A-yew Area: The main yew crate labels Mar 20, 2022
WorldSEnder
WorldSEnder requested changes Mar 20, 2022
View reviewed changes
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 mostly good to me. Approving, once I figure out if BTreeMap is a good decision. Looks like the right data structure, but I want an okay on the bundle size growth.

Also, you might know, how many render calls, previously skipped in RenderScheduler are now inherently skipped over?

packages/yew/src/scheduler.rs Outdated Show resolved Hide resolved
packages/yew/src/scheduler.rs Outdated Show resolved Hide resolved
voidpumpkin
voidpumpkin previously approved these changes Mar 20, 2022
View reviewed changes
@futursolo
Copy link
Member Author

Looks mostly good to me. Approving, once I figure out if BTreeMap is a good decision. Looks like the right data structure, but I want an okay on the bundle size growth.

I am not too worried about the BTreeMap size as it is also used by serde_json.

I don't think there is better way to achieve what this pull request is doing with smaller codebase than:

  • Compare ids on insertion by going through all items in a VecDeque 1 by 1.
    (Much slower on edge cases.)
  • Delegate sorting to JavaScript via js_sys.

(Vec::sort imports an entire quick sort implementation which is significantly bigger.)

Also, you might know, how many render calls, previously skipped in RenderScheduler are now inherently skipped over?

All previously skipped renders should still be skipped. The previous RenderScheduler delays render of a component until the position of the last time they asked to render. With BTreeMap you can register at most 1 render before a component is rendered.

WorldSEnder
WorldSEnder approved these changes Mar 21, 2022
View reviewed changes
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.

No more objections from me, then.

@futursolo futursolo merged commit 62d78d0 into yewstack:master Mar 21, 2022
@hamza1311
Copy link
Member

Just wondering, would it be possible to add test cases for this? The test would ensure the parent has a smaller id than the child

@futursolo
Copy link
Member Author

Just wondering, would it be possible to add test cases for this? The test would ensure the parent has a smaller id than the child

It's possible. Let's do this in a future pull request.

@futursolo futursolo deleted the comp-id branch March 26, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WorldSEnder WorldSEnder WorldSEnder approved these changes

@github-actions github-actions[bot] github-actions approved these changes

@voidpumpkin voidpumpkin voidpumpkin left review comments

Assignees
No one assigned
Labels
A-yew Area: The main yew crate performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@futursolo @hamza1311 @WorldSEnder @voidpumpkin