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.

Sign up for GitHub

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

Jump to bottom

Add inheritResolversFromInterfaces option - mergeSchemas #812

Merged
merged 2 commits into from
Jul 31, 2018
Merged

Add inheritResolversFromInterfaces option - mergeSchemas #812

merged 2 commits into from
Jul 31, 2018

Conversation

Druotic
Copy link
Contributor

@Druotic Druotic commented May 29, 2018 •
edited
Loading

This simply adds support for inheritResolversFromInterfaces via mergeSchemas (a simple passthrough thanks to the wonderful work done in pull request #720).

I didn't open an issue first since it was a pretty small change and I was looking for a quick way to achieve my goal, but I did link issue #616 below since this is just extending the concept to an additional util.

Let me know if this isn't in line with the original intent of that issue and/or the vision of the project, but I think it will prove to be a useful option.

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes) issue Types should inherit from interfaces when applying resolver map #616
  • 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
  • Update CHANGELOG.md with your change. Include a description of your change, link to PR (always) and issue (if applicable). Add your CHANGELOG entry under vNEXT. Do not create a new version number for your change yourself.

@ghost ghost added the feature New addition or enhancement to existing solutions label May 29, 2018
@Druotic Druotic changed the title Inherit interface resolvers merge schemas Add inheritResolversFromInterfaces option - mergeSchemas May 29, 2018
@ghost ghost added the feature New addition or enhancement to existing solutions label May 29, 2018
@Druotic
Copy link
Contributor Author

Druotic commented Jun 4, 2018

cc @reconbot @benjamn thoughts on this change? The change itself is pretty simple.

reconbot
reconbot approved these changes Jun 4, 2018
View reviewed changes
Copy link
Contributor

@reconbot reconbot left a comment

Choose a reason for hiding this comment

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

This looks great to me =)

src/test/testAlternateMergeSchemas.ts Outdated
@@ -10,6 +10,7 @@ import {
RenameRootFields,
} from '../transforms';
import { propertySchema, bookingSchema } from './testingSchemas';
//import { makeExecutableSchema } from '../schemaGenerator';
Copy link
Contributor

Choose a reason for hiding this comment

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

Left over from testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delayed response - I've been traveling all over the place recently. Yes, accidentally left this in. I'll remove it now.

@Druotic Druotic force-pushed the inherit-interface-resolvers-mergeSchemas branch from bec3dc2 to 09cd519 Compare June 11, 2018 01:39
@Druotic
Copy link
Contributor Author

Druotic commented Jun 11, 2018

Any chance of getting this into the next release @benjamn?

@Druotic
Copy link
Contributor Author

Druotic commented Jun 19, 2018

Resolved latest conflicts - I'd love to get this merged to avoid further conflicts. Anything else you'd like to see changed about this PR @benjamn @stubailo @freiksenet

@Druotic
Copy link
Contributor Author

Druotic commented Jun 26, 2018

FWIW, we've been using this in production for a while now with no issue.

@Druotic Druotic force-pushed the inherit-interface-resolvers-mergeSchemas branch from 4a72745 to e86bf77 Compare July 12, 2018 18:38
@Druotic
Copy link
Contributor Author

Druotic commented Jul 12, 2018

@benjamn @evans @abernix any chance this might could get reviewed soon? I've been pointing our prod deployments at my fork for a while now, with 1000s of users. I also recently put up another PR (PR #885) and will start using those changes in production, as well. I'd love to avoid maintaining a fork with multiple one-off contributions in addition to constant conflict resolution due to upstream changes.

Please let me know if there is anyone else I should CC here. I know you guys are probably very busy with your jobs and the proliferation of contributions to Apollo/GraphQL tooling, as well. Thanks again for all the effort that you've put into this library (and others)!

@Druotic
Copy link
Contributor Author

Druotic commented Jul 25, 2018

This PR will continue to have conflicts with master due to the CHANGELOG file. Once this PR is reviewed, I'll resolve conflicts.

@stubailo stubailo merged commit 1f2aead into ardatan:master Jul 31, 2018
@stubailo
Copy link
Contributor

Thank you @Druotic will release this today!

@Druotic
Copy link
Contributor Author

Druotic commented Jul 31, 2018

@stubailo woot! Thanks for the review/merges!

@dependencies dependencies bot mentioned this pull request Aug 2, 2018
Open
@dependencies dependencies bot mentioned this pull request Oct 1, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reconbot reconbot reconbot approved these changes

Assignees
No one assigned
Labels
feature New addition or enhancement to existing solutions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Druotic @stubailo @reconbot @jjbeavers-lifeomic