Open jerelmiller opened 4 months ago
A few thoughts:
from
. If this is intended to allow fragment-reading functions to work with fragments defined on non-normalized types, that would be great to capture in the RFC (and I would love it)!!useFragment
et. al when the passed fragment contains nested fragments. If that's the case, might also be good to call out, especially if that means fragments would also need to be tagged with @unmasked
during the transitionclient-preset
. It might be helpful to have a migration guide available.Are there any other ideas or suggestions on how to pass the parent object around beside React props? For example, if the component tree has a query at a top level but the colocated fragment isn't used for a series of nested children components? An example of this might be outside of the RFC here, but one of the things I like so much about useFragment
right now is how it has removed the need for us to pass data around via React props.
We'll either need add support to useFragment to allow it to be used without the cache, or we'll need to prevent usage with no-cache queries.
Would the proposed enhancement to from
potentially help solve for this case?
Something we've wanted for a while is automatic query registration for fragments so that a loading
flag (or suspense state) could be surfaced via useFragment
when any query containing that fragment is in flight. If you were doing that kind of registration, you could potentially warn if an attempt is made to use useFragment
with a no-cache query without passing the fragment data directly via the enhanced from
arg.
@adamesque thanks so much for the feedback!
If this is intended to allow fragment-reading functions to work with fragments defined on non-normalized types, that would be great to capture in the RFC
I would LOVE for this to be the case, though I'm still a bit technically uncertain if this is going to be possible.
I've added this as an open question to make sure this is captured. This is definitely going to be explored as a possibility.
I presume that data masking would also apply to data returned from
useFragment
et. al when the passed fragment contains nested fragments
YES! Thanks for calling this out! I've added this to the section on "What is data masking?" to make sure this is captured.
In our environment, graphql-codegen is widely used and has its own fragment masking features as part of its client-preset. It might be helpful to have a migration guide available.
Yes good call! Codegen compatibility is important, but capturing the differences between codegen's useFragment
helper and Apollo's built-in data masking will be important as well. Thanks for calling this out to make sure we are thinking through this as we develop the feature.
Would the proposed enhancement to
from
potentially help solve for this case?
Thats the goal, though really what I get stuck on is the "confusion" aspect of this potentially. Our docs start with marketing useFragment
as this:
The
useFragment
hook represents a lightweight live binding into the Apollo Client Cache
While its certainly possible that we change the "marketing strategy" on this hook, I just want to make sure that everything feels intuitive, otherwise we might end up with more frustration than solved problems.
I think this is just a matter of building it to work this way, then trying it out to see where the papercuts are. I plan to fully try out the new capabilities on our spotify showcase to make sure I can see it through the lens of the consumer. If all else fails, then we'll add a new hook to capture all of this functionality (though I'd prefer not to... naming is hard 🤣).
so that a loading flag (or suspense state) could be surfaced via useFragment when any query containing that fragment is in flight
This is planned as a followup to the data masking work 🙂. The data masking work should lay the ground work for us to determine when fragments are in flight so that we can layer in suspense functionality.
@yaboi thats a great question! Any updates we do to useFragment
will HAVE to be backwards compatible, otherwise its a breaking change. Everything you do today in terms of passing this around via context, reactive vars, etc. will continue to function as it does.
I really see the updates to from
as more of a "when you use it this additional way, it adds these behaviors". BUT this is a good callout because it should theoretically be possible to put that parent object that you'd pass via props into React context or a reactive var and still get the same behavior. I'll add this as an open question though to make sure its captured in the RFC.
Heyo, would love to see a small example of a unit test using the new from: option
Right now we're using cache.writeFragment + useFragment that just use an ID to introspect the cache.
In order to keep our tests atomic, its important we be able to mock the kind of object reference that goes into "from" easily
@blasterfiles thats a great callout! I've added https://github.com/apollographql/apollo-client/issues/11735 to track this. Thank you!
Hi! This looks really cool. One question I had is, what will the TypeScript types look for user
in this instance? How would one type "this prop is a thing I need to pass into useFragment", in a way that also ensures that the parent component passes in the right object?
We experimented a little bit with a custom @mask
directive, which works similar to @skip
at the cache reading level. This proved to be a pretty simple implementation (just a few line change in the readFromStore
) and it showed a nice performance boost - both for the initial useQuery
and subsequent updates (because unlike @nonreactive
the result of useQuery
isn't changing in optimism on fragment fields changes, so the cache doesn't even invoke watcher callback for parent useQuery
).
With this directive it was also trivial to adjust GraphQL codegen to exclude fragment fields from generated types. So, it could be easier to migrate exiting apps with such "opt-in" approach rather than "opt-out" with @unmasked
?
Funny enough, we still found @nonreactive
valuable for some of our scenarios where we wanted to keep some fields in the result of the parent useQuery
, while preventing them from triggering re-renders on data change.
@stubailo to be honest this is still a bit of an unknown right now. We are absolutely aware that we need TS types to align nicely with the updates to useFragment
here, but I cant't say we know exactly what that will look like quite yet. Stay tuned!
@vladar thats interesting to hear! Glad to hear you've had a good experience a masking-type solution! I'll be happy to have something first-class in the client as well here so that everyone can benefit 🙂.
So, it could be easier to migrate exiting apps with such "opt-in" approach rather than "opt-out" with
@unmasked
?
We've had discussions about the approach here and realize that an opt-out solution adds a bit of investment up-front, especially for large apps. I'll restate what I put in the RFC though on why we believe this is the right direction:
this approach has some advantages:
- New queries are automatically masked. This takes out possible human error forgetting to enable data masking on any newly introduced query to the application.
- Over time, you are removing code to adopt data masking rather than adding code. This makes it easy to spot which queries in your application have not yet adopted the feature.
We plan to make this process easier with migration tools so that its a more automated process. I strongly believe though the long-term benefits will outweight the up-front cost to enable it, even if your app remains in a state of "migration" for a long-period of time. This approach also prepares for a future where data-masking will become the default. This likely won't happen for another couple majors, but this is the long-term goal.
Funny enough, we still found
@nonreactive
valuable for some of our scenarios where we wanted to keep some fields in the result of the parent useQuery, while preventing them from triggering re-renders on data change.
That's very interesting! In my mind, data-masking will remove the need for @nonreactive
in most cases, so its cool to hear you've still found uses for it!
Absolutely love it!! Especially the thought that went into incremental adoption. I have a bunch of thoughts, mostly related to wording in this RFC, but some related to avoiding what I feel are mistakes that Relay made.
Overall, data masking is great and I think this is awesome, and I can't wait to see what y'all ship.
best_friend { name }
, and a follow up query selects best_friend { age }
and incidentally discovers a new best friend. Now, you have a fragment with missing data.
fragment Foo on Query { node(id: $id) { __typename }}
. The parent query is refetched with a new $id
. The Foo
fragment now has missing data and an outstanding query.network-only
is fetch yes and suspend yes, while store-and-network
is fetch yes, suspend no.@willAlwaysSuspendIfAnyDataIsMissing
, you can generate types that are non-optional. But if a fragment has @willNeverSuspend
, you can generate types that are optional. And then, enforce with the type system that you pass the appropriate suspensePolicy
to useFragment
.{ name, ...ChildFragment }
you might receive { name: 'John', [secretSymbol]: DenormalizedData }
@rbalicki2 first off, thank you so so much for this incredible feedback! There are definitely use cases that I hadn't quite thought of that will make excellent test cases. I'm going to take a bit of time and digest some of this and respond back with some of my thoughts when I've thought this through a bit more. I sincerely appreciate this!
@jerelmiller Thanks for your response, let me reply inline
We've had discussions about the approach here and realize that an opt-out solution adds a bit of investment up-front, especially for large apps.
For large apps it is indeed important to be able to experiment with the feature on a small subset of the codebase and once proven useful - gradually introduce it elsewhere. But I don't think opt-in vs. opt-out are mutually exclusive. In my mind the transition could be done in multiple stages:
@mask
directive). This will significantly simplify adoption for big apps, as it is a non-breaking change, doesn't require major upgrade, and allows people to experiment with it and "feel" the benefits.@mask
supported for selected parts of the app)@unmask
as the only way to opt-outSuch a gradual approach will make adoption much smoother and will also let you receive feedback earlier. Also, it doesn't conflict with the other stated benefits after major upgrade:
- New queries are automatically masked. This takes out possible human error forgetting to enable data masking on any newly introduced query to the application.
- Over time, you are removing code to adopt data masking rather than adding code. This makes it easy to spot which queries in your application have not yet adopted the feature.
Another challenge that would be great to cover in this RFC is manual writes to cache of operations containing masked fragments. This is a somewhat interesting topic, especially for strictly typed environments.
With data-masking reads and writes become non-symmetrical. Writes expect full data as input, whereas reads return partial data by default. This is a sort of ambiguity that could be resolved with different trade-offs.
(it is somewhat related to the section on manual cache.readQuery
and cache.readFragment
, but writes are even more tricky)
@vladar appreciate the observation on cache writes! I've added this as an open question for now to ensure this is captured. I think this will take a bit of exploration and experimentation to get right.
In my mind the transition could be done in multiple stages
I think you and I are aligned in the goals here which is ultimately to make data masking the default behavior. Where your thinking differs from mine is the approach to get there. My thinking was that changing the default essentially means flipping the dataMasking
flag from false
-> true
in a major version, then removing the flag altogether a major version after that. I worry that with your approach, its a bit of whiplash (i.e. first add @mask
, then in the next major, swap all those for @unmask
, though perhaps I'm misunderstanding your thinking here). The more friction we can avoid, the better. Doing the work up-front to globally enable and add @unmask
everywhere is a one-time up-front cost, then from there its deleting code to adopt the feature (more on that below).
That being said, you make a good point here:
it is indeed important to be able to experiment with the feature on a small subset of the codebase and once proven useful - gradually introduce it elsewhere
Experimentation can be important, especially if the codebase spans multiple teams (common in large enterprise organizations). Many of these teams likely have processes in place where major changes like this need a vetting period where perhaps a team lead can create a proof-of-concept and pitch it to the rest of the org. I definitely don't want to ignore this.
I've entertained the idea of allowing a @mask
directive as you mentioned, but I still have some reservations about this approach. Let me break down my points about this a bit further:
New queries are automatically masked.
This to me is a big one and where my goal is to try and ensure that once you've decided to adopt it, you're removing as much human error as possible. The @mask
approach only works if you remember to add that directive for any new query added to your system. If you or someone on your team forgets this directive, this can add more work after-the-fact as it means having to revisit this query, add the directive, and test to ensure that unmasked fields aren't used in the wrong places. Hopefully this could be caught in a code review, but again, we're all humans and prone to make mistakes (especially if you are under a time crunch trying to deliver on a feature).
It can be incredibly helpful if Apollo Client can help drive adoption naturally without much thought or process on your end.
Over time, you are removing code to adopt data masking rather than adding code
Again to reiterate, if your team adopts the feature by using @mask
rather than dataMasking: true
+ @unmask
, you're 1) adding code to incrementally adopt this feature and 2) once you've fully adopted data masking across all your queries, you're having to go back through and remove the @mask
directives in your existing queries. Furthermore, its difficult to tell at any given time whether or not you've actually fully adopted masking since it would require you to audit all your queries for a @mask
directive. Any query without a @mask
has the potential to break once you flip dataMasking
to true
.
This is what I mean by "removing code to adopt data masking". By default you're starting with @unmask
, so to mask a query, you're removing the directive. Once you're removed all @unmask
directives, there is no more work for you to do and you've fully adopted it across your codebase.
I find this approach easier to add linting rules to as well. During the transition period, you could set the linting rule to warn on @unmask
usage. Once you've fully adopted masking (i.e. you've removed all @unmask
directives), you switch that lint rule to error which helps prevent new queries from sneaking in @unmask
.
At the end of the day, my goal here with this approach is to try and help teams avoid mistakes when adopting this feature. I'm willing to admit that I might be trying to push too hard toward the "pit of success" though.
I'll entertain the idea of adding a @mask
directive or some other way to incrementally adopt in this manner. If we go that route, we'll likely recommend that you only use this for experimentation and prefer the @unmask
approach with all the reasons I listed above. Again, I do think your point about experimenting on a subset of queries to get a feel for it is important and can be useful to prove out adoption. I don't want to dismiss this idea.
Appreciate the insight and conversation here!
@jerelmiller I've opened a draft PR with reference implementation for @mask
in case if you want to play with it: https://github.com/apollographql/apollo-client/pull/11896
It could be used as a direct replacement for @nonreactive
directive.
This work is part of the Data masking milestone. Follow the milestone for progress on this feature.
Background
When building complex applications today, Apollo Client users reach for
useQuery
to get data into their components. This is no surprise as this is considered a best practice by the Apollo Client documentation.We are taking an initiative this year to change the recommendation to instead prefer fragment composition and fragment colocation as the standard pattern for building apps with Apollo Client. While the out-of-the-box experience with this pattern works, as recommended by our documentation, there are a few shortcomings of the existing solution:
To alleviate these shortcomings, we are are introducing data masking into Apollo Client. Data masking is popularized by Relay and is useful to avoid implicit dependencies between components.
What is data masking?
Data masking is the functionality that provides access only to the fields that were requested in the component. This prevents implicit coupling between components by allowing components to access only the fields requested by that component. For query components, this includes all fields in a query not included in a GraphQL fragment. For fragment components, this includes the fields defined in the fragment definition.
Take the following as an example:
In Apollo Client today, all
user
data is available to<App />
. This means that<App />
can access fields such asage
andbirthdate
, even though these fields were asked for by the fragment in Child. This creates an implicit coupling between<App />
and<User />
. For example, if<App />
consumeduser.age
, and<User />
was refactored to removeage
from the fragment, this would break the<App />
sinceage
will no longer be loaded by the query.Data masking solves this by only allowing the fields declared in that component to be accessible in the component that asked for that data. In this example,
<App />
would only have access touser.id
anduser.name
, but notuser.age
anduser.birthdate
since these were part of the fragment.<User />
would have access touser.age
anduser.birthdate
, but notuser.id
oruser.name
since these fields are not part of the fragment.The same applies to fragments that include nested fragments. One fragment cannot access data defined in a nested fragment. Take the following example:
Here
UserFragment
can accessid
andname
, but notage
andbirthdate
.UserProfileFragment
can accessage
andbirthdate
, but notid
andname
since these fields are not declared in the fragment.Usage
With data masking enabled, you will issue queries the same as you do today. This is typically done with the
useQuery
or theuseSuspenseQuery
hook if you've adopted Suspense. The difference is that you will not be able to access fields defined in fragments from these components.Instead, fields declared in fragments will be accessed through the
useFragment
hook*. To provide a nice developer experience, we plan update thefrom
option inuseFragment
to support passing the entire parent object that contains the fragment as the value to this option. See the "Example" section below for a full code sample.@defer
As part of this work, we will integrate
useFragment
with@defer
and detect when the fragment is in-flight. This will integrate with React Suspense and cause the component to suspend until the fragment data has loaded.Usage with non-suspenseful hooks
useFragment
will not be limited to usage with suspenseful hooks however (such asuseSuspenseQuery
). Users may not yet be compatible with Suspense or may primarily be usinguseQuery
to power their apps. Enabling suspense in these situations would be unwise and induce frustration.To avoid this, we plan to make
useFragment
aware of the hook that produced the query to conditionally suspend the component. This means that Suspense will only be available when the query is produced by a suspenseful hook such asuseSuspenseQuery
.Fetch policies with cached data
Users have the ability to leverage fetch policies to determine how to use cached data when consuming a query. For example, you can bypass the cache and force a network request with
network-only
, or read from the cache while fetching from the network withcache-and-network
.useFragment
should be aware of this to mimic the query hook behavior when determining whether to return a cached result or suspend. For example, when a deferred fragment is used within anetwork-only
query, the hook should suspend until the fragment is fulfilled, regardless of whether there is data in the cache. When used with acache-and-network
query,useFragment
should provide the cached data and rerender when the network request finishes.Non-React frameworks/libraries
The Apollo Client ecosystem is not limited to just React users. Libraries such as Apollo Angular and Vue Apollo provide view bindings for their respective view libraries. We plan to provide the foundation for these libraries to adopt this feature as well. This extends to users that use Apollo Client's core APIs.
We will be layering much of this work into Apollo's core query APIs, such as
watchQuery
and v3.10'swatchFragment
.Render performance
Adopting data masking will include more benefits than just programming best practices. It will also provide performance benefits.
Today, cache writes to fields in a fragment definition cause the query component to re-render, regardless of whether that component consumes the data from the fragment or not. Depending on the depth of the component tree mounted beneath the query component, this may have significant performance implications. Many users avoid this by introducing additional query hooks in components further down the component tree. This however comes at the cost of additional network requests.
You can avoid the render performance implications today with the use of the
@nonreactive
directive combined withuseFragment
[^3]. While this works well, it requires manual intervention.With data masking enabled, this performance benefit will be an out-of-the-box feature. Because
useFragment
will be required to read data out of a fragment, we will target re-renders on the fragment components directly when there are cache writes to fields in the fragment.Enabling this feature
Once this feature is introduced, it cannot be enabled automatically since this would constitute a breaking change. Instead, we will need to allow an opt-in to this feature. We plan to allow this in 2 ways:
Globally
We will allow data masking to be opted into globally by introducing a new option to
ApolloClient
. Enabling this option would automatically turn on data masking for every query that uses the client instance.We will recommend that new users and applications opt into this feature immediately as the default. Smaller apps that have the capacity to migrate in an afternoon should also consider enabling this feature. We plan to make this the default in future major versions of Apollo Client with the eventual goal to deprecate this option and make this standard behavior.
Incrementally
We understand that large apps cannot stop everything and adopt this feature in its entirely. To allow large apps to migrate over time, we will allow this feature to be opted into incrementally.
We will do so by first requiring that users enable data masking globally, then allowing users to opt-out of data masking per query. While this approach seems counterintuitive to the goal of an incremental migration, this approach has some advantages:
To make this approach feasible at scale, we will provide some out-of-the-box tools to handle the up-front work for you. For more information, see the “Migration tools” section below.
@unmask
We plan to add support for a new client-only directive
@unmask
that marks a query as unmasked. Any query marked with@unmask
will behave as it does today, allowing access to all fields, including those defined in fragments.There is one distinction however. When using
@unmask
, we will warn when accessing fields that would otherwise be masked if the directive were to be removed. This makes it easier to spot which fields will be affected by masking once the directive is removed from the query. When there are no more warnings, the directive should be safe to remove.Migration tools
We will provide utilities that will ease migration in large apps to make it feasible to adopt this feature.
We will provide a codemod that will crawl through the application and apply
@unmask
fields to every query. This will handle queries used ingql
tags and.graphql
/.gql
files.We will provide an ESLint plugin with a rule that will warn for queries that contain
@unmask
directives. This provides a more automated way to see areas of the codebase that have not yet adopted data masking. This also makes it possible to ban usage of the directive once data masking is fully adopted.Example
Open questions
Should we allow this for queries that use
no-cache
fetch policies?useFragment
is a cache API. It allows you to selectively read data out of the cache and re-render when that data changes. This gets tricky when used withno-cache
queries. We'll either need add support touseFragment
to allow it to be used without the cache, or we'll need to prevent usage withno-cache
queries.Depending on our final decision, this may warrant the introduction of a new hook to distinguish
useFragment
as a cache-only API.Should this feature apply to
cache.readQuery
andcache.readFragment
?Cache APIs, such as
cache.readQuery
andcache.readFragment
can be thought of as selectors for data in the cache. Data masking makes less sense here if you just need to read some arbitrary data out of the cache, especially since the queries/fragments you provide to these APIs do not actually have to be a query that was previously executed on the network. These APIs do not cause network requests and do not play a role in re-rendering your components.Instead, we may prefer to build this into the client layer between the cache and the end usage. For example, using
client.watchQuery
andclient.readQuery
would be data-masked. You'd pair this withclient.watchFragment
and/orclient.readFragment
to masked fragment data from these APIs. The inherent risk with this approach is that it may cause confusion since the distinction betweenclient.readQuery
andclient.cache.readQuery
may not be apparent.Can we allow fragment selections on non-normalized data?
Due to the way fragments work with the cache,
useFragment
is only able to read normalized data out of the cache. Thefrom
option creates the cache key used to look up the entity in the cache. With the planned update to thefrom
option, should it be possible to read non-normalized data viauseFragment
?The downside to allowing this is potential confusion on when this is allowed. Allowing non-normalized data to be selected when
from
originates from a query may make it feel like you can do this with any random fragment.Will it be possible to pass the parent objects provided to
from
in React context, reactive vars, etc.?It should theoretically be possible to pass around the parent objects that would normally be passed to child props in React context or other means of transporting values. I'm capturing this as an open question to make sure we are thinking about this while developing the feature.
How do cache writes work with data masking?
With data masking enabled, cache reads and writes are no longer symmetrical. We will need to explore ways to make
cache.writeQuery
make sense with this new paradigm.[^1]: Query components meaning components that initiate a GraphQL network request via a query hook, such as
useQuery
. [^2]: This can be avoided with the combination of@nonreactive
anduseFragment
today, but is more subject to error as it requires you to manually add@nonreactive
in the appropriate places. [^3]: For a more in-depth look at this feature, see @alessbell's blog post titled "Don’t Overreact! Introducing Apollo Client’s new@nonreactive
directive anduseFragment
hook".