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

Do not broadcast queries if Apollo state has not changed and if the previousState actually has anything inside of it. #226

Merged
merged 11 commits into from
May 25, 2016

Conversation

abhiaiyer91
Copy link
Contributor

@abhiaiyer91 abhiaiyer91 commented May 19, 2016

TODO:

  • Update CHANGELOG.md with your change
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@abhiaiyer91
Copy link
Contributor Author

actually i need to deepequal

@stubailo
Copy link
Contributor

Couldn't we just do === because of the immutable nature of the store? I feel like deepEqual-ing the entire store isn't necessary.

@abhiaiyer91
Copy link
Contributor Author

You may be right! I was just thinking that the store state is essentially

{
   queries,
   mutations,
   data
}

if we do a straight up === will we know if things actually changed? I guess I should have tested that.

@stubailo
Copy link
Contributor

Ohhhhh, wait you're right - I don't think we currently ensure that the old object is the same as the new one in our top-level reducer.

So for now let's do deepEqual and see what happens, we can optimize later. Maybe add a XXX comment or file a perf issue about it.

@abhiaiyer91
Copy link
Contributor Author

Ill add a comment and update the change log later

@stubailo
Copy link
Contributor

stubailo commented May 20, 2016

Btw we should probably add at least one test that checks that the observer doesn't fire if the result didn't change!

This is one of those things that is really easy to accidentally break.

@abhiaiyer91
Copy link
Contributor Author

Will do.

@coveralls
Copy link

coveralls commented May 25, 2016

Coverage Status

Coverage increased (+0.4%) to 93.053% when pulling 3f9a8a0 on StopTheDataMadness into 2ba9b95 on master.

@coveralls
Copy link

coveralls commented May 25, 2016

Coverage Status

Coverage increased (+0.08%) to 93.053% when pulling 4f0c612 on StopTheDataMadness into df0edf6 on master.

@abhiaiyer91
Copy link
Contributor Author

Yeeeeere


});

it('allows you to refetch queries with new variables', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How did this test get into this PR? Is this a new test or did it somehow get duplicated accidentally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WHOA DUPE

@stubailo
Copy link
Contributor

Looks awesome! Just confused about that extra test that seems unrelated.

@stubailo stubailo self-assigned this May 25, 2016
@stubailo
Copy link
Contributor

LGTM :shipit:

@stubailo
Copy link
Contributor

Great contribution dude!

jbaxleyiii pushed a commit that referenced this pull request Oct 17, 2017
Add documentation for Ember integration
jbaxleyiii pushed a commit that referenced this pull request Oct 18, 2017
update mutations.md formatting
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants