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.

Sign up for GitHub

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

Jump to bottom

add warning around missing __typename in fragment payload #413

Merged
merged 4 commits into from
Apr 22, 2019
Merged

add warning around missing __typename in fragment payload #413

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ff0165
add warning around missing __typename in fragment payload
finnigantime Apr 5, 2019
7059b38
PR feedback: make addTypename public and skip existing __typename check
finnigantime Apr 8, 2019
07319ee
check addTypeName in unit test
finnigantime Apr 8, 2019
1faf2f8
change to error and add unit test
finnigantime Apr 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/context/CacheContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,14 @@ export class CacheContext {
this._addTypename = config.addTypename || false;
}

/**
* Returns a boolean indicating whether this CacheContext has been
* configured to add __typename when transforming documents.
*/
getAddTypename() {
return this._addTypename;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than converting this into a getter, let's make the property public


/**
* Performs any transformations of operation documents.
*
Expand Down
7 changes: 7 additions & 0 deletions src/operations/SnapshotEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,13 @@ export class SnapshotEditor {
if (payload && payloadName in payload) {
warnings.push(`Encountered undefined at ${[...prefixPath, ...path].join('.')}. Treating as null`);
}

if (this._context.getAddTypename() && payloadName === '__typename') {
const existingTypenameValue = deepGet(this._getNodeData(containerId), path);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can skip this check - it's even more of a problem for cases where typename is missing (for some consumers of hermes, __typename is part of object identity)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. For unit testing, how to test? I didn't see an existing example checking tracer warnings. I see tests for catching InvalidPayloadError. By the way, should this be an InvalidPayloadError instead of a warning? That would be more visible to consumers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, failing hard here would be 👍

re: unit testing the tracer, we typically fall back to testing the console output, rather than the tracer itself (since that's more stable, and these messages are likely to change). But it's also not a very proven pattern; injecting the tracer might be better.

Example test: https://github.com/convoyinc/apollo-cache-hermes/blob/master/test/unit/Cache/transactions.ts#L53-L62

if (existingTypenameValue) {
warnings.push(`Encountered undefined payload value for __typename which will override __typename value on existing fragment.`);
}
}
}

let containerIdForField = containerId;
Expand Down