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

feat: allow for unmasking a fragment by means of a directive #28

Closed
wants to merge 4 commits into from

Conversation

JoviDeCroock
Copy link
Member

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 during parse but wanted to already surface this PR, would we be opposed to using visit from @graphql.web for this?

Copy link

changeset-bot bot commented Jan 19, 2024

🦋 Changeset detected

Latest 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

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<
Copy link
Member

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

Copy link
Member Author

@JoviDeCroock JoviDeCroock Jan 19, 2024

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

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

@JoviDeCroock

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.

Copy link
Member Author

@JoviDeCroock JoviDeCroock Jan 19, 2024

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

Copy link
Member

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

Copy link
Member Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Allow to use the automatic types without fragment refs/masks
2 participants