Skip to content

Commit

Permalink
Updated tests for #3182 (#3194)
Browse files Browse the repository at this point in the history 
* fix: ensure we check memos the first time a dependency uses them, even if the dependency always runs on its first run (closes #3181)

* Correct expected counter values down due to #3182

- As #3182 fixed the issue where superfluous resource fetches happened
  when hydration happened inside a nested component, the expected values
  for the counters are down to where they actually are supposed to be.

---------

Co-authored-by: Greg Johnston <[email protected]>
  • Loading branch information
@metatoaster @gbj
metatoaster and gbj authored Nov 5, 2024
1 parent 14e47e8 commit 8252655
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 47 deletions.
48 changes: 20 additions & 28 deletions examples/suspense_tests/e2e/features/check_instrumented_suspense_resource.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Feature: Using instrumented counters for real
And the following counters under section
| Server Calls (CSR) | |
| list_items | 0 |
| get_item | 2 |
| get_item | 1 |
| inspect_item_root | 0 |
| inspect_item_field | 0 |

Expand All @@ -42,8 +42,8 @@ Feature: Using instrumented counters for real
And the following counters under section
| Server Calls (CSR) | |
| list_items | 0 |
| get_item | 1 |
| inspect_item_root | 2 |
| get_item | 0 |
| inspect_item_root | 1 |
| inspect_item_field | 0 |

Scenario: Emulate step 7 of issue #2961
Expand All @@ -62,8 +62,8 @@ Feature: Using instrumented counters for real
And the following counters under section
| Server Calls (CSR) | |
| list_items | 0 |
| get_item | 1 |
| inspect_item_root | 3 |
| get_item | 0 |
| inspect_item_root | 2 |
| inspect_item_field | 0 |

Scenario: Emulate step 8, "not trigger double fetch".
Expand All @@ -82,7 +82,7 @@ Feature: Using instrumented counters for real
And the following counters under section
| Server Calls (CSR) | |
| list_items | 0 |
| get_item | 2 |
| get_item | 1 |
| inspect_item_root | 1 |
| inspect_item_field | 0 |

Expand All @@ -103,7 +103,7 @@ Feature: Using instrumented counters for real
And the following counters under section
| Server Calls (CSR) | |
| list_items | 0 |
| get_item | 3 |
| get_item | 2 |
| inspect_item_root | 1 |
| inspect_item_field | 0 |

Expand All @@ -124,21 +124,17 @@ Feature: Using instrumented counters for real
And the following counters under section
| Server Calls (CSR) | |
| list_items | 0 |
| get_item | 2 |
| get_item | 1 |
| inspect_item_root | 1 |
| inspect_item_field | 0 |

# Currently, get_item is invoked with `3` as the argument upon
# selection of `Item Listing` despite that `Item Listing` doesn't
# need `get_item` calls. Seems like it may be due to the system
# still reacting to the unmounting of the component that needed
# view that generated the original `Item 3` (hydrated from SSR).
# Tests above may also have this type of behavior, but is somewhat
# masked because the direction of going down and then back up, but
# if this behavior changes for the better (avoiding this spurious
# resource fetch) then the above tests may need updating to reflect
# the corrected behavior. Note the difference with the fully CSR
# scenario after this one
# The following tests previously showed the clear difference between
# hydration and CSR, where hydration resulting in extra server API
# calls via the resource while CSR did not suffer from the issue.
# With #3182 merged the issue is corrected, going up to components
# specified by the parent route should no longer result in the
# superfluous fetches for resources needed by component about to be
# unmounted.
Scenario: Emulate part of step 8 of issue #2961
Given I select the link Target 3##
And I refresh the page
Expand All @@ -147,12 +143,10 @@ Feature: Using instrumented counters for real
Then I see the following counters under section
| Server Calls (CSR) | |
| list_items | 0 |
| get_item | 1 |
| get_item | 0 |
| inspect_item_root | 0 |
| inspect_item_field | 0 |

# Instead of refreshing the page like above, CSR counters is reset
# instead to keep the starting counter conditions identical.
Scenario: Emulate above, instead of refresh page, reset csr counters
Given I select the link Target 3##
And I click on Reset CSR Counters
Expand All @@ -165,9 +159,7 @@ Feature: Using instrumented counters for real
| inspect_item_root | 0 |
| inspect_item_field | 0 |

# Again, the following two sets demostrates resources making stale
# and redundant requests when hydrated, and not do so when under
# CSR.
# Further two sets for good measure.
Scenario: Start with hydration from Target 41# and go up
Given I select the link Target 41#
And I refresh the page
Expand All @@ -177,11 +169,11 @@ Feature: Using instrumented counters for real
Then I see the following counters under section
| Server Calls (CSR) | |
| list_items | 0 |
| get_item | 1 |
| inspect_item_root | 1 |
| get_item | 0 |
| inspect_item_root | 0 |
| inspect_item_field | 0 |

Scenario: Start with hydration from Target 41# and go up
Scenario: Emulate the same csr counter reset, for Target 41#.
Given I select the link Target 41#
And I click on Reset CSR Counters
When I select the link Target 4##
Expand Down
3 changes: 3 additions & 0 deletions reactive_graph/src/computed/async_derived/arc_async_derived.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,9 @@ macro_rules! spawn_derived {
Some(ready_tx)
};

if was_ready {
first_run.take();
}
// begin loading eagerly but asynchronously, if not already loaded
if !was_ready {
any_subscriber.mark_dirty();
Expand Down
34 changes: 15 additions & 19 deletions reactive_graph/src/effect/effect.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,9 @@ impl Effect<LocalStorage> {

async move {
while rx.next().await.is_some() {
if first_run
|| subscriber.with_observer(|| {
subscriber.update_if_necessary()
})
if subscriber
.with_observer(|| subscriber.update_if_necessary())
|| first_run
{
first_run = false;
subscriber.clear_sources(&subscriber);
Expand Down Expand Up @@ -322,10 +321,9 @@ impl Effect<LocalStorage> {

async move {
while rx.next().await.is_some() {
if first_run
|| subscriber.with_observer(|| {
subscriber.update_if_necessary()
})
if subscriber
.with_observer(|| subscriber.update_if_necessary())
|| first_run
{
subscriber.clear_sources(&subscriber);

Expand Down Expand Up @@ -390,10 +388,9 @@ impl Effect<SyncStorage> {

async move {
while rx.next().await.is_some() {
if first_run
|| subscriber.with_observer(|| {
subscriber.update_if_necessary()
})
if subscriber
.with_observer(|| subscriber.update_if_necessary())
|| first_run
{
first_run = false;
subscriber.clear_sources(&subscriber);
Expand Down Expand Up @@ -437,9 +434,9 @@ impl Effect<SyncStorage> {

async move {
while rx.next().await.is_some() {
if first_run
|| subscriber
.with_observer(|| subscriber.update_if_necessary())
if subscriber
.with_observer(|| subscriber.update_if_necessary())
|| first_run
{
first_run = false;
subscriber.clear_sources(&subscriber);
Expand Down Expand Up @@ -490,10 +487,9 @@ impl Effect<SyncStorage> {

async move {
while rx.next().await.is_some() {
if first_run
|| subscriber.with_observer(|| {
subscriber.update_if_necessary()
})
if subscriber
.with_observer(|| subscriber.update_if_necessary())
|| first_run
{
subscriber.clear_sources(&subscriber);

Expand Down

0 comments on commit 8252655

Please sign in to comment.