[Data masking] Fix aggressive warnings when using objects marked with @unmask(mode: "migrate") with cache.identify
#12116
Conversation
✅ Docs Preview ReadyNo new or changed pages found. |
🦋 Changeset detectedLatest commit: c57ea51 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
size-limit report 📦
|
| // value is missing, otherwise our final result will contain additional | ||
| // properties that our original result did not have. This could happen with a | ||
| // deferred payload for example. | ||
| if (value === void 0) { |
There was a problem hiding this comment.
Something I realized when making this PR is that it was possible for us to add keys to the masked result that wasn't in the original data object. I felt this was incorrect and should preserve the original shape as much as possible (which means the same keys should be present/absent on the masked object). As such, I had to adjust a test to ensure we had data to properly test the warnings case.
@phryneas would love your input here to ensure this change makes sense.
| from: { __typename: "User", id: 1 }, | ||
| }); | ||
| const stream = new ObservableStream(observable); | ||
| client.writeFragment({ |
There was a problem hiding this comment.
As per my other comment, I had to make this change in the test since the keys are no longer present on the returned masked object since they weren't on the original data object (due to no data yet in the cache). This gets the test passing again.
jerelmiller
force-pushed
the
jerel/fix-agressive-warnings
branch
from
13b89b6 to
afd18c3
Compare
| @@ -2258,18 +2258,22 @@ describe("maskFragment", () => { | |||
|
|
|||
| const data = maskFragment( | |||
| deepFreeze({ | |||
| currentUser: { | |||
There was a problem hiding this comment.
This test was written incorrectly the first time 😅. This just happen to pass because we added accessor warnings for fields on the masked object that weren't present on the original data object. Yet another reason why I think the change not to add these properties unless they were on the original data object make sense.
src/cache/inmemory/policies.ts
Outdated
| @@ -392,7 +393,9 @@ export class Policies { | |||
| const policy = typename && this.getTypePolicy(typename); | |||
| let keyFn = (policy && policy.keyFn) || this.config.dataIdFromObject; | |||
| while (keyFn) { | |||
| const specifierOrId = keyFn({ ...object, ...storeObject }, context); | |||
| const specifierOrId = disableWarningsSlot.withValue(true, () => { | |||
There was a problem hiding this comment.
It might make sense to wrap this slot around the while loop, so if this iterates multiple times, the slot does less work.
There was a problem hiding this comment.
Good call! Updated in 0a6a3fb
Closes #12043
When passing an unmasked object in migration mode to
cache.identify, we'd get field accessor warnings even though the end user wasn't accessing them at all. This was due to the fact thatcache.identifydoes a spread operation of the result which shallow-copies all the fields, triggering the getters.This change adds a new
Slotthat we can use to disable field accessor warnings inside Apollo Client core for masked fields. This fixes thecache.identifycase as this is the only known case of the issue so far. In the future we can use thisSlotif we encounter other areas of the codebase that prematurely trigger the warnings.