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

RFC: Allow interfaces to implement other interfaces #2084

Merged
merged 3 commits into from
Oct 8, 2019
Merged

RFC: Allow interfaces to implement other interfaces #2084

merged 3 commits into from
Oct 8, 2019

Conversation

mike-marcacci
Copy link
Contributor

@mike-marcacci mike-marcacci commented Aug 13, 2019 •
edited
Loading

This is an implementation of the RFC described by spec PR graphql/graphql-spec#373

This is a successor to PR #1218 which had accumulated too many merge conflicts to bother fixing.

This was referenced Aug 13, 2019
Closed
Merged
@michaelstaib michaelstaib mentioned this pull request Aug 13, 2019
Closed
5 tasks
@mike-marcacci
Copy link
Contributor Author

It looks like I can't request particular reviewers, so I'm just tagging @mjmahone and @michaelstaib in case you weren't notified already. The PR is incredibly simple but touches many pieces of code (especially tests).

mjmahone
mjmahone reviewed Aug 14, 2019
View reviewed changes
Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

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

Overall this seems like a pretty minimal change for what we want to do. I like this direction a lot.

src/type/schema.js Show resolved Hide resolved
src/type/schema.js Outdated Show resolved Hide resolved
src/language/__tests__/schema-parser-test.js Show resolved Hide resolved
@mike-marcacci mike-marcacci force-pushed the rfc-interfaces-implement-interfaces-2019 branch from bf698a4 to 0f7e7ad Compare August 17, 2019 00:16
IvanGoncharov
IvanGoncharov reviewed Aug 21, 2019
View reviewed changes
src/type/schema.js Outdated Show resolved Hide resolved
@IvanGoncharov IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label Sep 12, 2019
IvanGoncharov
IvanGoncharov requested changes Sep 13, 2019
View reviewed changes
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

@mike-marcacci As agreed during WG there are two additional requirements:

  • Validation that prevents cycles in interface hierarchies
  • Blog post explaining motivation and scope of this feature.

@mike-marcacci If you want I can help with cycle validation? I think we can generalize code from #1359 to also validate interfaces.

@mike-marcacci
Copy link
Contributor Author

@IvanGoncharov I've added logic to prevent cyclic implementations. Because we already require flattening of transitively implemented interfaces, this ended up being trivial and didn't require any recursion. I believe this is in its final state.

I'm going to write up a blog post today, but this PR can probably be merged without it. Does anyone have a preference on how/where I publish the post? I can publish it myself on Medium after this is merged, or create a PR to the GraphQL blog.

@IvanGoncharov
Copy link
Member

Does anyone have a preference on how/where I publish the post? I can publish it myself on Medium after this is merged, or create a PR to the GraphQL blog

@mike-marcacci I think publishing it on personal account would be enough.

Also can you please update TS typings in tstypes folder?

I believe this is in its final state.

Great 🎉 I'm currently in the middle of review:
image
But I will try to finish it tomorrow.

@mike-marcacci
Copy link
Contributor Author

Wonderful! Great catch on the TS types; I just added those.

@IvanGoncharov
Copy link
Member

@mike-marcacci Currently we are in feature freeze for 14.x.x so this feature should be included in the upcoming 15.0.0. I plan to release 15.0.0-alpha.1 in the next few days and hopefully include this feature.

I branch of 14.x.x and merged 15.x.x into master. Can you please rebase this PR on top of master?

@mike-marcacci mike-marcacci force-pushed the rfc-interfaces-implement-interfaces-2019 branch from c3662d5 to f5ca91f Compare September 16, 2019 17:16
@mike-marcacci
Copy link
Contributor Author

Thanks so much for the thorough review @IvanGoncharov! I just rebased this on top of master. 👍

I also added a // prettier-ignore to prevent the addition of a trailing comma in one of the typedefs.

@IvanGoncharov
Copy link
Member

@mike-marcacci Sorry for the delay I was traveling to a conference and speaking today.
I will 100% finish the review tomorrow.

@mike-marcacci
Copy link
Contributor Author

@IvanGoncharov no worries at all! You're an absolute hero for all your work here :)

IvanGoncharov
IvanGoncharov reviewed Sep 25, 2019
View reviewed changes
src/type/validate.js Outdated Show resolved Hide resolved
@ruiaraujo ruiaraujo mentioned this pull request Feb 23, 2020
Open
@MichalLytek MichalLytek mentioned this pull request Apr 11, 2020
Closed
@redhead redhead mentioned this pull request Apr 17, 2020
Closed
@ivome ivome mentioned this pull request Apr 23, 2020
Closed
@andimarek andimarek mentioned this pull request Apr 29, 2020
Merged
@moberegger moberegger mentioned this pull request May 8, 2020
Closed
@mwinstanley mwinstanley mentioned this pull request May 18, 2020
Merged
@n1ru4l n1ru4l mentioned this pull request Jun 21, 2020
Merged
@Kingdutch Kingdutch mentioned this pull request Sep 25, 2020
Closed
@spawnia spawnia mentioned this pull request Nov 27, 2020
Merged
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 27, 2020
@Kingdutch
Fix extend implement interface in Parser
6d30db1 
This is part of the update to allow interfaces to implement interfaces.
A single extend statement to add an implementation of an interface
without field declarations is valid. This was caught by tests and brings
in a change from graphql/graphql-js#2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 27, 2020
@Kingdutch
Fix extend implement interface in Parser
5e4ab44 
This is part of the update to allow interfaces to implement interfaces.
A single extend statement to add an implementation of an interface
without field declarations is valid. This was caught by tests and brings
in a change from graphql/graphql-js#2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 27, 2020
@Kingdutch
Fix extend implement interface in Parser
951d882 
This is part of the update to allow interfaces to implement interfaces.
A single extend statement to add an implementation of an interface
without field declarations is valid. This was caught by tests and brings
in a change from graphql/graphql-js#2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
515a202 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
60a66b1 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
41f5990 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
c54f14b 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
b0ce9de 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
fe98d6f 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
4003548 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
0cdca81 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
90f3576 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
3242a38 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
aae3004 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
5d9dc7c 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
Kingdutch added a commit to Kingdutch/graphql-php that referenced this pull request Nov 30, 2020
@Kingdutch
Properly apply Schema changes for interface extension support
a8f94b6 
This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084
vladar pushed a commit to webonyx/graphql-php that referenced this pull request Jan 23, 2021
@Kingdutch @spawnia
Implement support for interfaces implementing interfaces (#740)
c4d2f82 
* Implement support for interfaces implementing interfaces

Closes #728

* Implement tests for interfaces implementing interfaces

This ports the JavaScript tests for `RFC: Allow interfaces to
implement other interfaces` to PHP. This should ensure that there is
sufficient test coverage for the changes made to support interfaces
implementing interfaces.

Tests taken from https://github.com/graphql/graphql-js/pull/2084/files
including any typoes in test description strings to aid in comparison.

* Fix extend implement interface in Parser

This is part of the update to allow interfaces to implement interfaces.
A single extend statement to add an implementation of an interface
without field declarations is valid. This was caught by tests and brings
in a change from graphql/graphql-js#2084

* Validate interface implemented ancestors

Part of the work done to implement interfaces implementing interfaces.
This was caught by test and improves on the previously done changes for
the SchemaValidationContext by implementing
`validateTypeImplementsAncestors` which was missing.

* Properly apply Schema changes for interface extension support

This redoes the work done for the Schema class since it was previously
guessed at. It now more closely follows graphql/graphql-js/pull/2084

* Improve interface extension related typehints

Co-authored-by: Benedikt Franke <[email protected]>

* Refine types

* Remove complex array shape type

* Don't remove but deprecate DANGEROUS_CHANGE_IMPLEMENTED_INTERFACE_ADDED

Removing a public constant is a breaking change and can not be
implemented in a minor version. Instead the internal value is changed to
ensure that existing code keeps working with the new interface
implementation logic.

* Don't remove but deprecate BREAKING_CHANGE_INTERFACE_REMOVED_FROM_OBJECT

Co-authored-by: Benedikt Franke <[email protected]>
Co-authored-by: Benedikt Franke <[email protected]>
@obi1kenobi obi1kenobi mentioned this pull request Mar 2, 2021
Closed
@imolorhe imolorhe mentioned this pull request Sep 17, 2021
Closed
@wilhelmeek wilhelmeek mentioned this pull request Nov 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjmahone mjmahone mjmahone left review comments

@IvanGoncharov IvanGoncharov Awaiting requested review from IvanGoncharov

Assignees
No one assigned
Labels
PR: breaking change 💥 implementation requires increase of "major" version number spec RFC Implementation of a proposed change to the GraphQL specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mike-marcacci @IvanGoncharov @mjmahone