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

Allow passing an array of resolver objects to makeExecutableSchema #576

Closed
wants to merge 11 commits into from
Closed

Allow passing an array of resolver objects to makeExecutableSchema #576

wants to merge 11 commits into from

Conversation

mfix22
Copy link
Contributor

@mfix22 mfix22 commented Jan 11, 2018

  1. Copying over lodash.merge and adding local type defs (this is my first time ever writing Typescript 😄)
  • Note: there are already 16 transient dependencies on other lodash modules
  • The issue here in not the copying (hehe), but rather writing the typedefs for what turns out is a 2000 line file (mostly comments albeit).
  1. Implementing similar functionality in mergeSchemas
  2. Updating CHANGELOG.md

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Closes Why should i merge resolvers manually when typedefs are automatically merge ?  #486
  • 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

@mfix22 mfix22 changed the title Allow passing an array of resolver functions to makeExecutable schema Allow passing an array of resolver objects to makeExecutableSchema Jan 11, 2018
@freiksenet freiksenet requested a review from stubailo January 11, 2018 08:42
Copy link
Contributor

@stubailo stubailo left a comment

Choose a reason for hiding this comment

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

This is a great idea but let's avoid using lodash.merge.

We should be able to get a similar result in a much simpler way by doing two loops over the input objects.

package.json Outdated
"deprecated-decorator": "^0.1.6",
"graphql-subscriptions": "^0.5.6",
"lodash.merge": "^4.6.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need lodash.merge - we could just write a simple function that loops through a few objects and merges them. Shouldn't be more than 10-20 lines of code I think since we don't need to support all the stuff that lodash.merge does.

We've intentionally kept the lodash dependency out.

Copy link
Contributor

Choose a reason for hiding this comment

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

freiksenet and others added 4 commits January 23, 2018 11:39
…es (#586)

Fixing the args with zero value or false
* Also recreate astNode for fields

In a [previous commit](fd9f626) we added the `astNode` property in the `reacreateCompositeType` function. That resulted in cache control working with schema stitching but only for GraphQL Types. By recreating the `astNode` prop also in `fieldToFieldConfig` cache control also works for fields. This is required for caching fields and hence queries.

* Add ast to input field node too
@mfix22
Copy link
Contributor Author

mfix22 commented Jan 23, 2018

cool thanks for the review guys! Let me know if you want me to move mergeDeep to it's own utility file.

@freiksenet
Copy link
Contributor

I've combined your two PRs and will merge them together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants