Attributes: Fix apply_diff_index_maps #2653

maurerdietmar · 2022-05-03T05:05:13Z

The old algorithm simply stops when key name/order changes, which is simply wrong.

WorldSEnder · 2022-05-03T07:03:11Z

Thanks for the PR. This seems to be present already in the initial implementation in #1537. It being IndexMap means that iteration order is defined by insertion order. I'd guess that has something to do with the bug lying undetected for some time.

Can you add test cases for the behaviour so that it doesn't pop up again later? Ask if you're not sure where and how to write those. Starting with the layout tests in dom_bundle/btag/mod.rs would be an option.

maurerdietmar · 2022-05-03T08:55:56Z

Thanks for the PR. This seems to be present already in the initial implementation in #1537. It being IndexMap means that iteration order is defined by insertion order. I'd guess that has something to do with the bug lying undetected for some time.

Can you add test cases for the behaviour so that it doesn't pop up again later? Ask if you're not sure where and how to write those. Starting with the layout tests in dom_bundle/btag/mod.rs would be an option.

How do I run those test? I get:

# cargo make test
[cargo-make] INFO - cargo make 0.35.11
[cargo-make] INFO - Build File: Makefile.toml
[cargo-make] INFO - Task: test
[cargo-make] INFO - Profile: development
[cargo-make] INFO - Running Task: workspace
/root/rust/yew-fork
[cargo-make][1] INFO - Project: yew
[cargo-make][1] INFO - Build File: Makefile.toml
[cargo-make][1] INFO - Task: test
[cargo-make][1] INFO - Profile: development
[cargo-make][1] INFO - Execute Command: "cargo" "test" "--features" "csr,ssr,hydration"
   Compiling yew v0.19.3 (/root/rust/yew-fork/packages/yew)
error[E0432]: unresolved import `yew::suspense::use_future`
  --> packages/yew/tests/hydration.rs:15:21
   |
15 | use yew::suspense::{use_future, Suspension, SuspensionResult};
   |                     ^^^^^^^^^^ no `use_future` in `suspense`

futursolo · 2022-05-03T08:59:26Z

How do I run those test? I get:

That test is missing a feature gate. You can add the following to the beginning of the file:

#![cfg(target_arch = "wasm32")]

WorldSEnder · 2022-05-03T09:00:32Z

The tests are run from the top-level folder with cargo make tests <-- plural

maurerdietmar · 2022-05-03T09:06:37Z

The tests are run from the top-level folder with cargo make tests <-- plural

cargo make tests

[cargo-make] INFO - cargo make 0.35.11
[cargo-make] INFO - Build File: Makefile.toml
[cargo-make] INFO - Task: tests
[cargo-make] INFO - Profile: development
[cargo-make] ERROR - Task tests not found
[cargo-make] WARN - Build Failed.

WorldSEnder · 2022-05-03T09:12:30Z

The tests are run from the top-level folder with cargo make tests <-- plural

cargo make tests

[cargo-make] INFO - cargo make 0.35.11 [cargo-make] INFO - Build File: Makefile.toml [cargo-make] INFO - Task: tests [cargo-make] INFO - Profile: development [cargo-make] ERROR - Task tests not found [cargo-make] WARN - Build Failed.

Oops, I had a version pre-#2640 checked out. You are right... I'll mentally add it to the pile related to #2641 and #2541 and the workaround by futursolo should deal for now ... (also need to add the workaround to hydration.rs, sorry for the bother)

maurerdietmar · 2022-05-03T09:15:39Z

How do I run those test? I get:

That test is missing a feature gate. You can add the following to the beginning of the file:

#![cfg(target_arch = "wasm32")]

OK, that fixes the bug.

Do you know how I cam run a single test? (make test always run all tests)

WorldSEnder · 2022-05-03T09:20:06Z

How do I run those test? I get:

That test is missing a feature gate. You can add the following to the beginning of the file:
#![cfg(target_arch = "wasm32")]

OK, that fixes the bug.

Do you know how I cam run a single test? (make test always run all tests)

You can go into packages/yew/Makefile.toml and temporarily apply (for native-test and wasm-test separately)

--- a/packages/yew/Makefile.toml
+++ b/packages/yew/Makefile.toml
@@ -1,6 +1,6 @@
 [tasks.native-test]
 command = "cargo"
-args = ["test", "--features", "csr,ssr,hydration"]
+args = ["test", "--features", "csr,ssr,hydration", "--", <your test filter>]
@@ -10,7 +10,9 @@ args = [
     "--headless",
     "--",
     "--features",
-    "wasm_test"
+    "wasm_test",
+    "--",
+    <your test filter, e.g. "index_maps">,
 ]

maurerdietmar · 2022-05-03T17:17:02Z

Finally managed to create a test case ...

github-actions · 2022-05-04T07:00:09Z

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

https://yew-rs-api--pr2653-fix-apply-diff-index-cq4lwish.web.app

_{(expires Wed, 11 May 2022 17:35:34 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

github-actions · 2022-05-04T07:14:11Z

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
boids	173.062	172.600	-0.462	-0.267%
contexts	110.644	110.260	-0.384	-0.347%
counter	86.920	86.458	-0.462	-0.531%
counter_functional	87.572	87.110	-0.462	-0.527%
dyn_create_destroy_apps	90.066	89.604	-0.462	-0.513%
file_upload	102.887	102.425	-0.462	-0.449%
function_memory_game	167.609	167.157	-0.452	-0.270%
function_router	350.646	350.262	-0.384	-0.109%
function_todomvc	162.228	161.775	-0.452	-0.279%
futures	226.463	225.938	-0.525	-0.232%
game_of_life	107.747	107.295	-0.452	-0.420%
inner_html	83.957	83.495	-0.462	-0.550%
js_callback	113.063	112.680	-0.384	-0.339%
keyed_list	195.296	194.834	-0.462	-0.237%
mount_point	86.546	86.084	-0.462	-0.534%
nested_list	116.081	115.697	-0.384	-0.331%
node_refs	90.718	90.256	-0.462	-0.509%
password_strength	1538.550	1538.088	-0.462	-0.030%
portals	97.382	96.998	-0.384	-0.394%
router	316.220	315.836	-0.384	-0.121%
simple_ssr	494.152	493.695	-0.457	-0.092%
ssr_router	426.188	425.731	-0.457	-0.107%
suspense	110.765	110.376	-0.389	-0.351%
timer	89.658	89.196	-0.462	-0.515%
todomvc	143.289	142.837	-0.452	-0.316%
two_apps	87.554	87.092	-0.462	-0.528%
web_worker_fib	153.592	153.130	-0.462	-0.301%
webgl	87.638	87.176	-0.462	-0.527%

✅ None of the examples has changed their size significantly.

WorldSEnder

Performance alert is a false positive, as far as I can tell.

futursolo · 2022-05-04T12:39:18Z

@maurerdietmar Could you please rebase / merge from master so size comparison test can run?

The old algorithm simply stops when key name/order changes, which is simply wrong. Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>

WorldSEnder

Arguably, both the new and old attribute set often have <10 entries, so all those iterations are preferable to keeping extra state to track leftover attributes to remove (changing the signature slightly, alternatively old can potentially be mutable). We can try it in a follow-up but the fix itself looks good. Thanks for the test case.

WorldSEnder added the A-yew Area: The main yew crate label May 3, 2022

maurerdietmar added a commit to maurerdietmar/yew that referenced this pull request May 4, 2022

add test case for test for bug yewstack#2653

3b54e46

maurerdietmar added a commit to maurerdietmar/yew that referenced this pull request May 4, 2022

add test case for test for bug yewstack#2653

aa705b8

maurerdietmar force-pushed the fix-apply-diff-index-maps branch from 3b54e46 to aa705b8 Compare May 4, 2022 08:10

WorldSEnder previously approved these changes May 4, 2022

View reviewed changes

maurerdietmar added 2 commits May 4, 2022 16:54

Attributes: Fix apply_diff_index_maps

946b439

The old algorithm simply stops when key name/order changes, which is simply wrong. Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>

add test case for test for bug yewstack#2653

a986c03

maurerdietmar dismissed WorldSEnder’s stale review via a986c03 May 4, 2022 14:55

maurerdietmar force-pushed the fix-apply-diff-index-maps branch from aa705b8 to a986c03 Compare May 4, 2022 14:55

futursolo approved these changes May 5, 2022

View reviewed changes

WorldSEnder approved these changes May 5, 2022

View reviewed changes

futursolo merged commit e588013 into yewstack:master May 5, 2022

maurerdietmar deleted the fix-apply-diff-index-maps branch May 5, 2022 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attributes: Fix apply_diff_index_maps #2653

Attributes: Fix apply_diff_index_maps #2653

maurerdietmar commented May 3, 2022

WorldSEnder commented May 3, 2022

maurerdietmar commented May 3, 2022

futursolo commented May 3, 2022

WorldSEnder commented May 3, 2022 •

edited

maurerdietmar commented May 3, 2022

WorldSEnder commented May 3, 2022 •

edited

cargo make tests

maurerdietmar commented May 3, 2022

WorldSEnder commented May 3, 2022 •

edited

maurerdietmar commented May 3, 2022 •

edited

github-actions bot commented May 4, 2022 •

edited

github-actions bot commented May 4, 2022 •

edited

WorldSEnder left a comment

futursolo commented May 4, 2022

WorldSEnder left a comment

Attributes: Fix apply_diff_index_maps #2653

Attributes: Fix apply_diff_index_maps #2653

Conversation

maurerdietmar commented May 3, 2022

WorldSEnder commented May 3, 2022

maurerdietmar commented May 3, 2022

futursolo commented May 3, 2022

WorldSEnder commented May 3, 2022 • edited

maurerdietmar commented May 3, 2022

cargo make tests

WorldSEnder commented May 3, 2022 • edited

cargo make tests

maurerdietmar commented May 3, 2022

WorldSEnder commented May 3, 2022 • edited

maurerdietmar commented May 3, 2022 • edited

github-actions bot commented May 4, 2022 • edited

github-actions bot commented May 4, 2022 • edited

Size Comparison

WorldSEnder left a comment

Choose a reason for hiding this comment

futursolo commented May 4, 2022

WorldSEnder left a comment

Choose a reason for hiding this comment

WorldSEnder commented May 3, 2022 •

edited

WorldSEnder commented May 3, 2022 •

edited

WorldSEnder commented May 3, 2022 •

edited

maurerdietmar commented May 3, 2022 •

edited

github-actions bot commented May 4, 2022 •

edited

github-actions bot commented May 4, 2022 •

edited