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

Fix: @attr defaultValue() results should persist after initialization #9355

Merged
merged 5 commits into from
May 10, 2024
Merged

Fix: @attr defaultValue() results should persist after initialization #9355

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
41c6e0c
defaultValue function results should persist after initialization
Apr 12, 2024
d788966
defaultValue function results should persist after initialization
Apr 12, 2024
2e38bed
clear default values on patch changes
Apr 30, 2024
f96cb82
defaultValue caching test
Apr 30, 2024
444511f
Merge branch 'main' into fix/default-value-attr-persists
christophersansone May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/json-api/src/-private/cache.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we also need to clear defaultAttrs during patchLocalAttributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the quick feedback!

I reviewed patchLocalAttributes and happy to make the change, but feel like I need a bit more clarification. FWICT, the purpose of that method is to simply remove the key/value pairs from localAttrs that are not in remoteAttrs or inflightAttrs. I'm assuming this would occur when, say, a record is updated from the server while there are still local changes? If so, should we also have similar logic to remove any defaultAttrs that have keys present in localAttrs, remoteAttrs, or inflightAttrs?

The way I see it, yes it might be worthwhile to ensure defaultAttrs is kept clean at every possible chance, but because the order of operations of getAttr would inherently prevent a default value from being exposed if any of the other sets of attributes contains that key, it could be considered superfluous.

In thinking through the scenario, if I understand it correctly:

  • A record is fetched remotely that does not define an attribute with a defaultValue function, so the default value is created.
  • The record is returned from the server again, only this time it does have a value for that, thereby masking the previous default value.
  • The record is returned from the server, yet without the attribute once again. What should happen?

This seems like quite an edge case, but I think my opinion would be that the original default value would take effect again, as opposed to a new one being created.

Happy to do what you think makes the most sense here, just let me know!

Also, I was hoping to find a nice set of unit tests for this cache class, but no luck 😕. Do you have a recommendation regarding where to write some tests for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like quite an edge case, but I think my opinion would be that the original default value would take effect again, as opposed to a new one being created.

If the server has previously informed us of a state, and that state later disappears, imo the correct thing to do is start over.

I was hoping to find a nice set of unit tests for this cache class, but no luck

The cache requires integration tests. Newest ones are here https://github.com/emberjs/data/tree/main/tests/ember-data__json-api/tests/integration/cache and here https://github.com/emberjs/data/blob/main/tests/main/tests/integration/cache/json-api-cache-test.js

but most of them are scattered through the main test app, with most having been written from the perspective of user observable behaviors (due largely to how cache grew out of Store/Model/InternalModel).

I'd recommend adding new tests at that first link above since this is a behavior specific to JSON:API cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

@christophersansone got any more time for this one? I can poke at it a bit if needed

Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ interface CachedResource {
id: string | null;
remoteAttrs: Record<string, Value | undefined> | null;
localAttrs: Record<string, Value | undefined> | null;
defaultAttrs: Record<string, Value | undefined> | null;
inflightAttrs: Record<string, Value | undefined> | null;
changes: Record<string, [Value | undefined, Value]> | null;
errors: JsonApiError[] | null;
Expand All @@ -90,6 +91,7 @@ function makeCache(): CachedResource {
id: null,
remoteAttrs: null,
localAttrs: null,
defaultAttrs: null,
inflightAttrs: null,
changes: null,
errors: null,
Expand Down Expand Up @@ -1036,6 +1038,7 @@ export default class JSONAPICache implements Cache {
// we report as `isEmpty` during teardown.
cached.localAttrs = null;
cached.remoteAttrs = null;
cached.defaultAttrs = null;
cached.inflightAttrs = null;

const relatedIdentifiers = _allRelatedIdentifiers(storeWrapper, identifier);
Expand Down Expand Up @@ -1096,11 +1099,18 @@ export default class JSONAPICache implements Cache {
return cached.inflightAttrs[attr];
} else if (cached.remoteAttrs && attr in cached.remoteAttrs) {
return cached.remoteAttrs[attr];
} else if (cached.defaultAttrs && attr in cached.defaultAttrs) {
return cached.defaultAttrs[attr];
} else {
const attrSchema = this._capabilities.schema.fields(identifier).get(attr);

upgradeCapabilities(this._capabilities);
return getDefaultValue(attrSchema, identifier, this._capabilities._store);
const defaultValue = getDefaultValue(attrSchema, identifier, this._capabilities._store);
if (typeof attrSchema?.options?.defaultValue === 'function') {
cached.defaultAttrs = cached.defaultAttrs || (Object.create(null) as Record<string, Value>);
cached.defaultAttrs[attr] = defaultValue;
}
return defaultValue;
}
}

Expand Down Expand Up @@ -1133,6 +1143,10 @@ export default class JSONAPICache implements Cache {
delete cached.changes![attr];
}

if (cached.defaultAttrs && attr in cached.defaultAttrs) {
delete cached.defaultAttrs[attr];
}

this._capabilities.notifyChange(identifier, 'attributes', attr);
}

Expand Down Expand Up @@ -1195,6 +1209,7 @@ export default class JSONAPICache implements Cache {
}

cached.inflightAttrs = null;
cached.defaultAttrs = null;

if (cached.errors) {
cached.errors = null;
Expand Down