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: ensure we check memos the first time a dependency uses them, even if the dependency always runs on its first run (closes #3181) #3182

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gbj
Copy link
Collaborator

@gbj gbj commented Nov 1, 2024

No description provided.

@gbj
Copy link
Collaborator Author

gbj commented Nov 4, 2024

@metatoaster I'm trying to figure out the failing tests on suspense_tests on this branch. When I test manually I don't see the issue, but I'm not sure I understand the tests you added well enough to understand what's going wrong here. When you have a bit of time, could you help me try to figure out what's going on?

@metatoaster
Copy link
Contributor

metatoaster commented Nov 5, 2024

Some of the tests I've added basically did document the existing behavior, not the ideal behavior, which I did comment in the test itself - in this test I was documenting the existing behavior where on departure from a component, the resource it needed was retrieved despite how it was on the verge of being disposed when hydrated, and while under CSR this wasn't an issue. Now both versions show the same expected behavior where this extra unnecessary fetch is no longer necessary, and that's a good thing. Moreover, this fix did correct other extra fetches which means those other tests need to pare down the counts, and again this is a very good thing.

However, the concerning part is the base test case is picking up what appears to be the SSR counters from the CSR navigation, and its only for that particular test, which is weird, this is what I am more concerned about. I will get to the bottom of this, but in the mean time I am going to fix those tests.

Edit: Looking at the test further it might appear that the browser for whatever reason isn't loading the wasm quick enough for that failure run, I put in a new pull request on top of this one (actually just one that included all the changes with this one), and the tests passed.

metatoaster added a commit to metatoaster/leptos that referenced this pull request Nov 5, 2024
- As leptos-rs#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.
benwis pushed a commit that referenced this pull request Nov 5, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants