Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Update lastResult whenever data changes to ensure components re-rende… #2840

Merged
merged 4 commits into from
Mar 20, 2019

Conversation

Glennrs
Copy link
Contributor

@Glennrs Glennrs commented Mar 5, 2019

I've been unable to write a test case to reproduce this scenario, however, the logic is fairly simple to follow. I'm happy to be informed if I'm missing something and this PR is not required. Scenario is:

  1. Component renders with value from cache, which sets lastResult.
  2. Value of cache is set to another value. Component re-renders.
  3. Value is set back to the original value, therefore shallowEquals does not pass since lastResult is fixed to the original render.

This PR ensures that lastResult is updated if set, when new data is available.

Fixes #2857.
Fixes #2887.

…r if result is set back to original lastResult
@Glennrs Glennrs requested review from benjamn and hwillson as code owners March 5, 2019 16:15
@apollo-cla
Copy link

@Glennrs: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@hwillson
Copy link
Member

Thanks for submitting this PR @Glennrs. We can't accept this PR as it stands currently, as we have no way to verify that it's fixing a problem. A test showing what this fixes (to help avoid regressions), or a small runnable reproduction that demonstrates how this is a problem, would definitely help. I'll close this for now, but to help push the discussion forward, I'd suggest opening a new issue with as many details as you can provide that shows how this is a problem. Once we've been able to confirm this is an issue, we'll then be ready to accept a PR. Thanks!

@hwillson hwillson closed this Mar 10, 2019
@Glennrs
Copy link
Contributor Author

Glennrs commented Mar 10, 2019

The following Gist has a repro for the issue. You can see that after the Query component has its properties updated, it will set lastResult to true and subsequently always early out when the value is set back to true again (meaning it remains in a state where it renders false).

https://gist.github.com/Glennrs/8051fcac26ec50a8d113cf4e4586769c

@hwillson
Copy link
Member

Re-opening as this issue has been demonstrated by the reproduction in #2887. I'll add a regression test, then get this merged.

@hwillson
Copy link
Member

I've updated the original PR message to reference a few issues this will help fix, but I'm sure there are others that this will help with (either directly or indirectly), like #1931.

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

All systems go here @Glennrs - thanks very much!

@yloeza
Copy link

yloeza commented Mar 22, 2019

@hwillson when are you guys planning to release this? is it going to be just a patch version?

@mac-s-g
Copy link

mac-s-g commented Apr 8, 2019

also hoping to test this out!
sitting at 2.4.1 until it's available

@mac-s-g
Copy link

mac-s-g commented Apr 8, 2019

update:
i'm running into some other issues with 2.4.1. so, i gotta move back to 2.5.4 and wait for the release. loading states will be a little clunky til then.

maybe a good time to give a shout out to all the contributors on this project.
it's an AWESOME module. really amazing work. thank you for creating and maintaining it!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants