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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds support for removing a field on a particular type #964

Closed
wants to merge 2 commits into from
Closed

Adds support for removing a field on a particular type #964

wants to merge 2 commits into from

Conversation

orta
Copy link
Contributor

@orta orta commented Oct 2, 2018

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • 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.

馃憢 - I couldn't find a way to remove a field from a type using the existing transformers, so I added one!

Is there a better way to do this?

@ghost ghost added the feature New addition or enhancement to existing solutions label Oct 2, 2018
orta
orta commented Oct 2, 2018
View reviewed changes
src/transforms/RemoveFieldsOnType.ts
// extensionASTNodes: type.extensionASTNodes,
// interfaces: type.getInterfaces(),
// isTypeOf: type.isTypeOf,
fields: newFields
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the general implementation from TransformRootFields but it didn't include any of the commented options in the type, should I add them here or remove my comments?

@orta orta mentioned this pull request Oct 3, 2018
Closed
@ashkan18 ashkan18 mentioned this pull request Jan 15, 2019
Merged
@yaacovCR
Copy link
Collaborator

yaacovCR commented Apr 8, 2019 •
edited

Fixes #819

@yaacovCR yaacovCR mentioned this pull request Apr 8, 2019
Closed
@the-noob
Copy link

Any reason for not merging this ?

@dhudec-asurint
Copy link

I also would love to see this merged. Is there a timeline planned for releasing it?

@yaacovCR yaacovCR mentioned this pull request May 2, 2019
Closed
@yaacovCR yaacovCR mentioned this pull request May 21, 2019
Closed
5 tasks
@macrozone
Copy link

macrozone commented Jul 20, 2019 •
edited

looks complete, can we have it merged?

Also is there a workaround or external package to use? i tried to copy the change into my application, but it uses some internals that are not exposed by graphql-tools.

@yaacovCR
Copy link
Collaborator

yaacovCR commented Jul 21, 2019 •
edited

@macrozone, available in https://www.npmjs.com/package/graphql-tools-fork

@yaacovCR
Copy link
Collaborator

Closing as fork includes this fix, so will be merged in with #1307.

@yaacovCR yaacovCR closed this Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.

None yet
5 participants
@orta @yaacovCR @the-noob @dhudec-asurint @macrozone