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

Make ChildProps.data non-optional #1143

Merged
merged 1 commit into from Sep 26, 2017

Conversation

Projects
None yet
4 participants
@choffmeister
Copy link
Contributor

choffmeister commented Sep 26, 2017

Please see issue #1083

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Sep 26, 2017

@choffmeister: 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/

@choffmeister choffmeister changed the title Make ChildProps.data non-optional (#1083) Make ChildProps.data non-optional Sep 26, 2017

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Sep 26, 2017

Warnings
⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Sep 26, 2017

@choffmeister choffmeister force-pushed the choffmeister:childprops-data branch from 19bbddb to 33da21b Sep 26, 2017

@choffmeister

This comment has been minimized.

Copy link
Contributor Author

choffmeister commented Sep 26, 2017

True thx. Still there are tests failing which is weired, since I did not change any actual code. Also the tests suite does also not run through locally on master branch...

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Sep 26, 2017

The build fail seems to be related to some typescript checks that now fail because of the changed types: https://travis-ci.org/apollographql/react-apollo/builds/279954551#L773

@choffmeister

This comment has been minimized.

Copy link
Contributor Author

choffmeister commented Sep 26, 2017

OK, these error is from making data optional also on OptionProps. Before there was no typing error but just some unit tests failed. Will have to look into it.

@choffmeister choffmeister force-pushed the choffmeister:childprops-data branch from 33da21b to 5bb797a Sep 26, 2017

@choffmeister

This comment has been minimized.

Copy link
Contributor Author

choffmeister commented Sep 26, 2017

@leoasis I reverted the part for OptionProps so this is at least correct for now. I am also not really sure that OptionProps.data is indeed optional. At least how it is implemented now it is not.

EDIT: Yes OptionProps.data is optional, since either .data or .mutate is filled.

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Sep 26, 2017

Hmm I think data can be optional in both cases, since you won't get it either in ChildProps when you set up a mutation.

@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Sep 26, 2017

I think this one is a bit hard to typecheck, since the decision on wether this HoC acts as a mutation or as a query, really depends on the graphql Document that is passed to it, and it's determined at runtime.

Anyway, at least the other type should be Partial since that won't come when data is loading.

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Sep 26, 2017

I had no idea this even existed! So cool!

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Sep 26, 2017

hmm these test failures seem to be everywhere? I wonder what it is. Anyhow, its not related to this PR so I'll merge this through!

@jbaxleyiii jbaxleyiii merged commit 2138994 into apollographql:master Sep 26, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
CLA Author has signed the Meteor CLA.
Details
Danger ⚠️ Danger found some issues. Don't worry, everything is fixable.
Details
bundlesize ./dist/index.min.js: 11.95kB < maxSize 12kB gzip
Details
@leoasis

This comment has been minimized.

Copy link
Collaborator

leoasis commented Sep 26, 2017

@jbaxleyiii but won't data be undefined if we just use graphql for a mutation? I guess that why it was optional in the first place.

Also, I think OptionProps here https://github.com/apollographql/react-apollo/pull/1143/files#diff-41df78d86ed4de80aae9b0b9560feb2bL65 should have the same Partial treatment to TResult

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment