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

Rework traversal strategy in Policies#applyMerges. #5880

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jan 29, 2020

Previously, I thought it was important to process any child object fields with custom merge functions before processing their parents, which required a post-order traversal of the result tree (in other words, calling policies.applyMerges recursively on incoming.__value before invoking the custom merge function). I have come to realize this was a mistake, for a few different and somewhat subtle reasons.

First, a merge function should be able to return merged data in any format (specified by the TExisting type), but it should also be able to assume the incoming data has not been modified by merge functions. Reasoning about data types is much simpler if the incoming data is always just the type of data returned by the GraphQL server/schema (StoreValue, most generically), and the existing type is always some other type chosen by the developer (TExisting), possibly but not necessarily the same as the schema type (for example, an indexed lookup table, or an ordered history of results). Calling policies.applyMerges recursively before invoking the merge function violated this assumption, because nested incoming data could be turned into TExisting data before the merge function saw it.

Note: this is not just a case of trying to make the type system happy, because TypeScript's flexible type inference isn't powerful enough to provide any reliable warnings or errors here. A stronger static type system would make enforcement easier, but even in a completely dynamic language this kind of type-level reasoning would still be important, because it has real semantic implications.

Second, there's no universal way for the cache to merge arrays automatically (should it replace, concatenate, and/or deduplicate? what if the other value is not an array?), so blindly calling policies.applyMerges on array values before the merge function had a chance to process them was a risky plunge. This commit severs the false linkage between items in different arrays that happen to have the same index.

Third, I have been hoping to provide an options.merge helper function to facilitate forced merges of unidentified data, like { ...existing, ...incoming } but with appropriate regard for custom merge functions. Unfortunately, the risk of repeatedly merging already merged data would have required that all merge functions be idempotent (a useful quality for most merge functions to have, but not always possible). I feared that implementing options.merge was going to require a new field function in addition to read and merge (like merge but taking two TExisting values and merging them idempotently), but thankfully the changes in this commit allow us to implement that functionality with just read and merge.

Previously, I thought it was important to process any child object fields
with custom merge functions before processing their parents, which
required a post-order traversal of the result tree (in other words,
calling policies.applyMerges recursively on incoming.__value *before*
invoking the custom merge function). I have come to realize this was a
mistake, for a few different and somewhat subtle reasons.

First, a merge function should be able to return merged data in any format
(specified by the TExisting type), but it should also be able to assume
the incoming data has not been modified by merge functions. Reasoning
about data types is much simpler if the incoming data is always just the
type of data returned by the GraphQL server/schema (StoreValue, most
generally), and the existing type is always some other type chosen by the
developer (TExisting), possibly but not necessarily the same as the schema
type (for example, an indexed lookup table, or an ordered history of
results). Calling policies.applyMerges recursively *before* invoking the
merge function violated this assumption, because nested incoming data
could be turned into TExisting data before the merge function saw it.

Note: this is not just a case of trying to make the type system happy,
because TypeScript's flexible type inference isn't powerful enough to
provide any reliable warnings or errors. A stronger static type system
would make enforcement easier, but even in a completely dynamic language
this kind of type-level reasoning would still be important, because it has
actual semantic implications.

Second, there's no universal way for the cache to merge arrays
automatically (should it replace, concatenate, and/or deduplicate? what if
the other value is not an array?), so blindly calling policies.applyMerges
on array values before the merge function had a chance to process them was
a risky plunge. This commit severs the false linkage between items in
different arrays that happen to have the same index.

Third, I have been hoping to provide an options.merge helper function to
facilitate forced merges of unidentified data, like { ...existing,
...incoming } but with appropriate regard for custom merge functions.
Unfortunately, the risk of repeatedly merging already merged data would
have required that all merge functions be idempotent (a useful quality for
most merge functions to have, but not always possible). I feared that
implementing options.merge was going to require a new field function in
addition to read and merge (like merge but taking two TExisting values and
merging them idempotently), but thankfully the changes in this commit
allow us to implement that functionality with just read and merge.
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Looks great @benjamn! 🧙‍♂️

@benjamn benjamn merged commit 87b9fd2 into master Jan 30, 2020
@benjamn benjamn deleted the rework-applyMerges-traversal-strategy branch January 30, 2020 17:58
benjamn added a commit that referenced this pull request Sep 24, 2020
Instead of recursively searching for FieldValueToBeMerged wrapper objects
anywhere in the incoming data, processSelectionSet and processFieldValue
can build a sparse tree specifying just the paths of fields that need to
be merged, and then applyMerges can use that tree to traverse only the
parts of the data where merge functions need to be called.

These changes effectively revert #5880, since the idea of giving merge
functions a chance to transform their child data before calling nested
merge functions no longer makes as much sense. Instead, applyMerges will
be recursively called on the child data before parent merge functions run,
the way it used to be (before #5880).
benjamn added a commit that referenced this pull request Sep 24, 2020
Instead of recursively searching for FieldValueToBeMerged wrapper objects
anywhere in the incoming data, processSelectionSet and processFieldValue
can build a sparse tree specifying just the paths of fields that need to
be merged, and then applyMerges can use that tree to traverse only the
parts of the data where merge functions need to be called.

These changes effectively revert #5880, since the idea of giving merge
functions a chance to transform their child data before calling nested
merge functions no longer makes as much sense. Instead, applyMerges will
be recursively called on the child data before parent merge functions run,
the way it used to be (before #5880).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants