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

re-export collectFields #3246

Closed
wants to merge 2 commits into from
Closed

re-export collectFields #3246

wants to merge 2 commits into from

Conversation

PabloSzx
Copy link
Contributor

No description provided.

@saihaj saihaj added the PR: bug fix 🐞 requires increase of "patch" version number label Aug 26, 2021
@ardatan
Copy link
Member

ardatan commented Aug 28, 2021

Actually it would be nice to export collectFields directly from index file for the future.

@saihaj
Copy link
Member

saihaj commented Aug 29, 2021

Actually it would be nice to export collectFields directly from index file for the future.

That would be better. One more breaking change but I think we should do this now

@ardatan
Copy link
Member

ardatan commented Aug 29, 2021

@saihaj If we both export collectFields from execute and index file, that won't be a breaking change, no?

@saihaj
Copy link
Member

saihaj commented Aug 29, 2021

Yep 👍🏼

@PabloSzx
Copy link
Contributor Author

Yep 👍🏼

done 👍

@IvanGoncharov
Copy link
Member

@PabloSzx Thanks for PR collectFields was internal function.
So v15 and v16 have different prototypes (last 2 arguments have different types) so they are incompatible anyway even if we export them.
I'm totally ok with making this function part of public API but before that, I need to do some changes to it.

@PabloSzx
Copy link
Contributor Author

@ardatan who is the lead maintainer of graphql-tools should be more interested in that comment

@robrichard
Copy link
Contributor

Calling out here that the experimental defer/stream branch also changes the api of collectFields.

https://github.com/graphql/graphql-js/pull/2839/files#diff-b17f02a116457aa7749e04b3f306eba462f95979d4f76c313a6073fa201e4fe4R52

I think we need to carefully consider the api of this function before making it external, so that we can support defer/stream without introducing additional breaking changes.

@PabloSzx PabloSzx closed this Sep 23, 2021
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Sep 23, 2021
IvanGoncharov added a commit that referenced this pull request Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants