-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add cached, named Visibility profiles #5074
Merged
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
fcfed4c
Add cached, named Visibility profiles
rmosolgo 5830fb6
Start visible_in
rmosolgo 792bfb7
Fix test
rmosolgo a00bbbe
update object shape testS
rmosolgo 865cd9b
Merge branch 'master' into named-visibility
rmosolgo 3d6b1ae
update shape tests again
rmosolgo d52198b
Fix tests
rmosolgo bb05e15
Fix vis initialization
rmosolgo 213253e
update test
rmosolgo 37f5081
Use a positional arg for Ruby 2.7 compat
rmosolgo 122272d
Fix some tests
rmosolgo 22bec21
Fix name in migration mode
rmosolgo d05874f
Update ruby version on lint
rmosolgo 3c0eb04
Lint fixes
rmosolgo 3c367e5
Merge branch 'master' into named-visibility
rmosolgo b9932a0
Remove visible_in
rmosolgo 23c79bb
Rename Subset => Profile
rmosolgo 466a4e6
Start documenting Visibility::Profile
rmosolgo f591d98
Update test
rmosolgo 31787c6
Merge branch 'master' into named-visibility
rmosolgo aa90ecc
Merge branch 'master' into named-visibility
rmosolgo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rmosolgo – could you provide any more detail here? This memory would really help finding the horcruxes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derp... sorry! I think it must have to do with this: f591d98
I'm going to take a stroll down memory lane and if I can remember or rediscover the issue, I'll update this doc and follow up here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I updated the note here: 4d325ef
I think it's possible that this compatibility issue could be resolved now, because
Schema::Visibility
does have a top-level map of types, created by this code here:graphql-ruby/lib/graphql/schema/visibility.rb
Line 203 in 4d325ef
If it looks like your schema is affected by this issue, let me know and I'll give it a shot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invaluable context, thanks for the quick reply. I'm going to read that new sentence on repeat this afternoon until it clicks. Also, my gosh – abstracts are hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! If I've understood it properly, a minimal situation is like this:
Going from old to new, I think the difference in visible schema would be this:
Visibility
wouldn't checkSomeUnion
and discoverThing
-- butWarden
would, because it used the top-level map fromSchema.types
. To acheive compatibility, you'd have to add:Or add
field :thing, Thing
toQuery
-- just some way to discoverThing
while traversing visible members of the schema.But this might be fixable now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant by that ... sorry for the frenzied terseness :P
orphan_types
is specifically for the case when an object type is only used in the schema as an implementer of an interface. It's a practical consideration because, in Rails development, we need Rails to load the Object type class so that GraphQL-Ruby can know about the interface implementation. Otherwise, the file might never be loaded.For example, in a schema like this:
GraphQL-Ruby would need
Node.orphan_types(Thing)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last question then... how does
Schema.orphan_types
differ fromInterface.orphan_types
? I haven't always used the later but haven't noticed unexpected side effects. Is schema orphan types all-encompassing? Thanks again, this is all incredibly helpful (both for visibility upgrade and gem projects).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two
orphan_types
methods are basically the same: just ways to attach things to the schema that might not be attached. I don't know of any differences between them 😅 IIRC the gem doesn't even require than an Interface's orphan types implement that interface ... Yes, always usingSchema.orphan_types
is a fine approach --Schema.from_definition
just passes all object types to that method!graphql-ruby/lib/graphql/schema/build_from_definition.rb
Line 135 in 4d325ef
Happy to help 🍻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema.from_definition
proceeds to do the same work again with the interfaces themselves directly below. I think that could be removed entirely (I omitted it from the stitching composer and it seems to work?).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch -- I'll try removing it in #5269