kloeckner-i / ember-cli-mirage-graphql

A library for mocking GraphQL with Ember CLI Mirage
MIT License
24 stars 10 forks source link

Union type check #41

Closed miwialex closed 4 years ago

miwialex commented 4 years ago

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 commented 4 years ago

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

miwialex commented 4 years ago

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 commented 4 years ago

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

jneurock commented 4 years ago

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

jgwhite commented 4 years ago

https://github.com/miwialex/ember-cli-mirage-graphql/pull/4 ← hoping this will get the tests green across the matrix

jgwhite commented 4 years ago
Screen Shot 2020-02-13 at 17 21 50

argghh

jgwhite commented 4 years ago

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

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

jneurock commented 4 years ago

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

jgwhite commented 4 years ago

Is there still an issue with the build passing?

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

jneurock commented 4 years ago

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

jgwhite commented 4 years ago

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

jgwhite commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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!