-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: allow for unmasking a fragment by means of a directive #28
Conversation
🦋 Changeset detectedLatest commit: 1306412 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ee9bb83
to
a499abf
Compare
a499abf
to
8feba9d
Compare
src/selection.ts
Outdated
? Node['name']['value'] extends keyof Fragments | ||
? Fragments[Node['name']['value']] extends FragmentDefDecorationLike | ||
? makeFragmentRef<Fragments[Node['name']['value']]> | ||
? isUnmaskedFragmentRec<Node['directives']> extends true | ||
? getSelection< |
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.
I think, we can probably replace $tada.fragmentDef
, and instead of it being set to the definition, set it to either the fragment result type or makeFragmentRef
.
This way, this part of the code doesn't have to have any knowledge of what it's embedding.
Furthermore, since $tada.fragmentDef
would then be equivalent to ResultOf<typeof fragment>
, this part of the code doesn't have to do any work 🎉
This does require us to place @mask_disable
on the fragment definition, but we could do:
fragment FragName on Type @_noMasking {
# ...
}
This then in turn has the benefit of parse
only having to check the first directive and filtering the directives
array on the FragmentDefinition
nodes it pushes
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.
I thought about this and locally defined fragments already work out just fine so would this only be for fragments that we define externally outside of a component? If so this might be a tad limiting 😅 I don't mind tbh just thinking out loud, my reasoning was that on the spread this gives a lot of freedom
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.
Small note, since I was mistaken,
updating $tada.fragmentDef
’s type wouldn't be enough, since we only use the definition node via getFragmentsOfDocumentsRec
.
So, we'd only have to update getFragmentsOfDocumentsRec
and annotate that. It's a bit messy, since fragmentId
is “just on there”. But we could remove $tada.fragmentId
and replace getFragmentsOfDocumentsRec
’s output to add a new annotation, ad-hoc.
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.
work out just fine so would this only be for fragments that we define externally outside of a component
yea, see note above, since my implementation path was flawed. We could update getFragmentsOfDocumentsRec
to actually add the annotation that contains the type (either fragment ref or result type)
I know that this gives a lot more freedom on the spread, but I'd argue it'd have to be on the fragment definition anyway, otherwise FragmentOf
couldn't switch to giving you the ResultOf
-type, i.e. if we put it on the spread then the consuming component would have to have knowledge of how to define its input type.
This is of course possible, but, if we think of it as if it manipulated the runtime data, by making this configurable on the spread, there's an inconsistency between what's accepted and what's passed.
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.
I think I made it work 😅 it works in the example, let me add it to the docs 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.
re. docs, do you think we should write a general “Fragment Composition” page under a “Guides” section and mention it there?
I was thinking that a general “Fragment Composition” next to a future “Typed Documents” page (which would explain TypedDocumentNode
) could explain fragment composition, masking, and component colocation.
We could also call it “Fragment Colocation” to be fair
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.
Hmm, should I refrain from documentation until we get the hollistic topic then?
c060958
to
edd6fbc
Compare
edd6fbc
to
dc6b3c8
Compare
dc6b3c8
to
1306412
Compare
Resolves #23
Summary
This adds the possibility of annotating a fragment-spread with
@mask_disable
I went for the same name as houdini for familiarity. This still needs some code to remove these directives duringparse
but wanted to already surface this PR, would we be opposed to usingvisit
from@graphql.web
for this?