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

Rework a bunch of cfg(feature) flags to be more principled #2666

Merged
merged 10 commits into from May 7, 2022

Conversation

WorldSEnder
Copy link
Member

@WorldSEnder WorldSEnder commented May 7, 2022

Description

  • spawn_local is now always present, so Suspension::from_future etc is not feature/platform gated anyway. The scheduler for async panics on unsupported platforms. We could go for not compiling instead when ssr without a specific platform like tokio is enabled. Having to write any(target_arch = .., feature = .. list all supported platforms) is a no-go for me. Just do what wasm_bindgen does.
  • doc_test and wasm_test have been removed. The former without replacement, cfg(doctest) is stable if one needs it. The latter replaced with target_arch = "wasm32".
  • trace_hydration removed. Instead, a way to get the lifecycle logs for a component was re-introduced. We should consider moving to a principled log solution, like the log crate, after measuring impact on code size instead?
  • in some places, the features have been grouped in feat_ submodules and imports moved into there from top-level. Honestly lifecycle.rs is still a bit of a mess, but this PR is a start towards Feature soundness is a chore to deal with #2641

$ rg . -e "feature|target_arch =" --count-matches -U | awk -F: '{sum += $2} END {print sum}'
- 540
+ 499

Checklist

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

github-actions[bot]
github-actions bot previously approved these changes May 7, 2022
@github-actions
Copy link

github-actions bot commented May 7, 2022

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

https://yew-rs-api--pr2666-universal-scheduler-3dtmbqa6.web.app

(expires Sat, 14 May 2022 14:56:37 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

github-actions[bot]
github-actions bot previously approved these changes May 7, 2022
@github-actions
Copy link

github-actions bot commented May 7, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 172.566 172.764 +0.197 +0.114%
contexts 109.438 109.633 +0.195 +0.178%
counter 86.458 86.653 +0.195 +0.226%
counter_functional 87.109 87.305 +0.195 +0.224%
dyn_create_destroy_apps 89.576 89.771 +0.195 +0.218%
file_upload 102.426 102.622 +0.196 +0.192%
function_memory_game 167.141 167.338 +0.197 +0.118%
function_router 350.237 350.424 +0.187 +0.053%
function_todomvc 161.787 161.983 +0.196 +0.121%
futures 226.421 226.617 +0.196 +0.087%
game_of_life 107.315 107.512 +0.196 +0.183%
inner_html 83.492 83.688 +0.195 +0.234%
js_callback 112.682 112.877 +0.195 +0.173%
keyed_list 194.837 195.033 +0.196 +0.101%
mount_point 86.085 86.280 +0.195 +0.227%
nested_list 115.716 115.911 +0.195 +0.169%
node_refs 90.252 90.447 +0.195 +0.216%
password_strength 1539.377 1539.575 +0.198 +0.013%
portals 96.999 97.194 +0.195 +0.201%
router 319.274 319.472 +0.197 +0.062%
simple_ssr 493.991 494.015 +0.023 +0.005%
ssr_router 425.462 425.602 +0.140 +0.033%
suspense 110.373 110.524 +0.151 +0.137%
timer 89.165 89.360 +0.195 +0.219%
todomvc 142.857 143.054 +0.196 +0.137%
two_apps 87.091 87.286 +0.195 +0.224%
web_worker_fib 153.139 153.335 +0.196 +0.128%
webgl 87.177 87.373 +0.196 +0.225%

✅ None of the examples has changed their size significantly.

github-actions[bot]
github-actions bot previously approved these changes May 7, 2022
leads to bundle size increase, so stick to the optimized version
github-actions[bot]
github-actions bot previously approved these changes May 7, 2022
Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

PR looks good. Just a couple of changes to provide a better experience

We should consider moving to a principled log solution, like the log crate, after measuring impact on code size instead?

This is something I brought up before but it never got anywhere: we don't need to. The browser offers a way to filter logs in the console. We need to log at proper levels and let the browser handle the rest.

We can't provide LOG environment variable so if we end up using log, a recompile would be needed to change the log levels

examples/function_router/src/bin/function_router.rs Outdated Show resolved Hide resolved
packages/yew/Cargo.toml Outdated Show resolved Hide resolved
packages/yew/Makefile.toml Outdated Show resolved Hide resolved
packages/yew/src/io_coop.rs Show resolved Hide resolved
@hamza1311 hamza1311 added A-yew Area: The main yew crate A-yew-router Area: The yew-router crate labels May 7, 2022
@hamza1311 hamza1311 added this to the v0.20 milestone May 7, 2022
github-actions[bot]
github-actions bot previously approved these changes May 7, 2022
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.

Could you please also update the Supported Features section in lib.rs to reflect the current status of features?

packages/yew/src/html/component/scope.rs Show resolved Hide resolved
packages/yew/src/html/component/scope.rs Show resolved Hide resolved
packages/yew/src/dom_bundle/mod.rs Outdated Show resolved Hide resolved
packages/yew/src/io_coop.rs Show resolved Hide resolved
packages/yew/src/html/component/scope.rs Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes May 7, 2022
@WorldSEnder
Copy link
Member Author

WorldSEnder commented May 7, 2022

The failing test cases are unrelated and failing due to missing synchronization points in the ssr tests and the browser being a bit slow. Regarding the log, will maybe discussed in an upcoming issue/PR, but unrelated for now.

@hamza1311 hamza1311 merged commit 1d579a8 into yewstack:master May 7, 2022
@WorldSEnder WorldSEnder deleted the universal-scheduler branch May 7, 2022 16:52
@futursolo futursolo mentioned this pull request Jun 25, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate A-yew-router Area: The yew-router crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants