Union type check #41

miwialex · 2020-02-05T14:46:51Z

Currently in the getFieldType function, the only check that happens is to see if the field is __typename and then returns null. Otherwise it automatically looks for the field under type._fields which isn't going to be there for the type GraphQLUnionType. I added a check for this, and then added some logic to find the appropriate field for the GraphQLUnionType. Note: This does not add full support for the union types yet, but I was able to write a custom fieldsMap to resolve my union type to support my needs currently. This should unblock people for now to be able to use union types

jneurock · 2020-02-05T15:03:33Z

Cool. Thanks for adding this! I think it needs a test, though. Can you add one that will only pass with your code changes?

chadian · 2020-02-05T15:33:16Z

addon/utils.js

@@ -22,6 +22,14 @@ export const ensureList = (item) => item == null

 export const getFirstKey = (obj) => Object.keys(obj)[0];

+export const getUnionField = (type, fieldName) => {
+  for (const t of type._types) {
+    if (t._fields[fieldName]) {


This might be an edge case, but would there be an issue if multiple types had the same fieldName but the fields were different types?

That would be an issue but addressing it seems like a much larger piece of work that can potentially come later.

Yes, this would be an issue. I feel the appropriate thing would be to add full UnionType functionality at that point, where this is a simple fix that allows us to add UnionTypes and use mapping to accomplish it

miwialex · 2020-02-12T14:52:53Z

Cool. Thanks for adding this! I think it needs a test, though. Can you add one that will only pass with your code changes?

@jneurock @jgwhite was nice enough to add some tests :)

jgwhite · 2020-02-12T17:26:01Z

Parts of the build matrix are failing. I’ll dig in tomorrow and try to figure out why.

jneurock · 2020-02-12T17:29:19Z

I'll try to help when I get a chance. I want to get this merged and released as soon as I can.

jgwhite · 2020-02-13T11:36:30Z

miwialex#4 ← hoping this will get the tests green across the matrix

jgwhite · 2020-02-13T16:22:42Z

argghh

jgwhite · 2020-02-14T11:33:51Z

@jneurock this PR is in a bit of an awkward spot.

For the acceptance test of union type handling, we need to subclass ember-apollo-client’s apollo service in the dummy app.
To do this, we need to install ember-native-class-polyfill.
The polyfill only supports Ember >= 3.4.
Because of this, ember-apollo-client >= 2.0.0 only supports ember >= 3.4.

I’d encourage us to adopt the same supported range as ember-apollo-client (rather than the full LTS range), but totally open to discussion.

jneurock · 2020-02-14T12:34:40Z

@jgwhite, without giving a ton of thought, I'd lean the same way regarding supported Ember versions. This addon should be GraphQL client agnostic, though. I'll have to think about it a little more but my first thought is to release these changes with a 0.3.0 version of the addon and update the README to add a note about this stuff. IDK how many people are using this addon with pre-3.0 Ember, if anyone at all.

jgwhite · 2020-02-14T12:57:29Z

IDK how many people are using this addon with pre-3.0 Ember, if anyone at all.

I highly suspect nobody will be in this position.

I think cutting a 0.3.0 is the right approach. And we wouldn’t have to prevent the addon working with older Ember versions, just say that we don’t officially support anything < 3.4.0.

jgwhite · 2020-02-18T15:11:46Z

@jneurock what do you think about cutting an 0.3.0 this week?

jneurock · 2020-02-18T16:49:15Z

I'm happy to release ASAP. Is there still an issue with the build passing?

jgwhite · 2020-02-18T17:23:36Z

Is there still an issue with the build passing?

The build is passing for the proposed new support range or Ember >= 3.4

jneurock · 2020-02-18T17:33:34Z

I see. The Ember Try setup can be changed in this pull request.

jgwhite · 2020-02-18T18:35:43Z

The Ember Try setup can be changed in this pull request.

Yeah, the PR does tweak the ember try config but maybe there’s some Travis tweaking still needed

add check for union type to properly get field type

This commit is, I’ll confess, controversial. However, ember-apollo-client only supports Ember >= 3.4 and it makes our lives *much* easier to follow suit, and to take that even further and follow ember-cli’s recommendation to only test the last two LTS releases. The real source of the 3.4 minimum is ember-native-class-polyfill which ember-apollo-client now requires Ember < 3.6. As per [the ember-apollo-client 2.0.0 release notes][1]: > If you are using Ember versions 3.4 or 3.5 you must add > ember-native-class-polyfill to your application. [1]: https://github.com/ember-graphql/ember-apollo-client/releases/tag/v2.0.0

jgwhite · 2020-02-19T14:53:00Z

@jneurock this is green now. I faffed around with the ember-try config a bunch and ended up just using exactly what’s in the current ember-cli addon blueprint (the last two LTS releases). I think we should still support back to 3.4 (as discussed) but this seemed like a stable starting point.

jneurock · 2020-02-19T15:48:56Z

Cool. Thanks! At one point I tried and failed to update dependencies a while back. Everything worked locally but 3.4 failed in Travis and I felt too close to madness to continue. I'll think a little bit on the support topic and how to appropriately update the README but I think it could be fine to release a new version that works with these changes.

jgwhite · 2020-02-25T14:58:32Z

@jneurock let us know what you’re thinking with this and if there’s any way we can help out with the release process.

jneurock

What's this about? Just curious.

tests/dummy/app/services/apollo.js

jneurock · 2020-02-25T20:07:00Z

@jgwhite I was looking into running more tests with older Ember versions but I think it doesn't matter that much. This is really about the dummy app in the addon and not the versions of Ember that are supported. I had a couple of questions about some of the file changes but other than that, this is ready to go. Maybe after a little explanation of some changes I'll quickly release 0.3.0 as we discussed and update the README a bit.

Where I could probably use some help is documenting how someone would go about using this addon with union types in the README. Any help there would be appreciated. I have no real world union type experience to help me there. Thanks!

miwialex requested a review from jneurock February 5, 2020 14:53

chadian reviewed Feb 5, 2020

View reviewed changes

miwialex and others added 9 commits February 19, 2020 15:33

union type check

81b820d

add check for union type to properly get field type

Use IntrospectionFragmentMatcher in dummy app

ba31ac1

Add eq helper to dummy app

9d25c9c

Add acceptance test for union type handling

9a33c8e

Switch apollo service to classic ember class

94a0afa

Add ember-native-class-polyfill in dev

721a989

Upgrade version of ember-apollo-client

5c64d85

Use latest ember-apollo-client best practises

591238c

Quotes 🤦‍♀️

dc4d11c

jgwhite force-pushed the union-type-check branch from af8ff06 to fe175bc Compare February 19, 2020 14:33

jgwhite force-pushed the union-type-check branch from fe175bc to 7e75372 Compare February 19, 2020 14:38

jneurock reviewed Feb 25, 2020

View reviewed changes

tests/dummy/app/services/apollo.js Show resolved Hide resolved

jneurock merged commit 4f811c2 into kloeckner-i:master Feb 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Union type check #41

Union type check #41

miwialex commented Feb 5, 2020

jneurock commented Feb 5, 2020

chadian Feb 5, 2020

jgwhite Feb 11, 2020

miwialex Feb 12, 2020

miwialex commented Feb 12, 2020

jgwhite commented Feb 12, 2020

jneurock commented Feb 12, 2020 via email •

edited

jgwhite commented Feb 13, 2020 •

edited

jgwhite commented Feb 13, 2020

jgwhite commented Feb 14, 2020

jneurock commented Feb 14, 2020

jgwhite commented Feb 14, 2020 •

edited

jgwhite commented Feb 18, 2020

jneurock commented Feb 18, 2020

jgwhite commented Feb 18, 2020

jneurock commented Feb 18, 2020 via email •

edited

jgwhite commented Feb 18, 2020

jgwhite commented Feb 19, 2020 •

edited

jneurock commented Feb 19, 2020 via email •

edited

jgwhite commented Feb 25, 2020

jneurock left a comment

jneurock commented Feb 25, 2020

Union type check #41

Union type check #41

Conversation

miwialex commented Feb 5, 2020

jneurock commented Feb 5, 2020

chadian Feb 5, 2020

Choose a reason for hiding this comment

jgwhite Feb 11, 2020

Choose a reason for hiding this comment

miwialex Feb 12, 2020

Choose a reason for hiding this comment

miwialex commented Feb 12, 2020

jgwhite commented Feb 12, 2020

jneurock commented Feb 12, 2020 via email • edited

jgwhite commented Feb 13, 2020 • edited

jgwhite commented Feb 13, 2020

jgwhite commented Feb 14, 2020

jneurock commented Feb 14, 2020

jgwhite commented Feb 14, 2020 • edited

jgwhite commented Feb 18, 2020

jneurock commented Feb 18, 2020

jgwhite commented Feb 18, 2020

jneurock commented Feb 18, 2020 via email • edited

jgwhite commented Feb 18, 2020

jgwhite commented Feb 19, 2020 • edited

jneurock commented Feb 19, 2020 via email • edited

jgwhite commented Feb 25, 2020

jneurock left a comment

Choose a reason for hiding this comment

jneurock commented Feb 25, 2020

jneurock commented Feb 12, 2020 via email •

edited

jgwhite commented Feb 13, 2020 •

edited

jgwhite commented Feb 14, 2020 •

edited

jneurock commented Feb 18, 2020 via email •

edited

jgwhite commented Feb 19, 2020 •

edited

jneurock commented Feb 19, 2020 via email •

edited