fix: Don't notify changes for attributes not registered with the schema #9698
+87
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gitKrystan
added
🎯 canary
gitKrystan
changed the title
runspired
approved these changes
Feb 25, 2025
runspired
approved these changes
Feb 25, 2025
gitKrystan
commented
Feb 25, 2025
| @@ -557,10 +557,12 @@ export default class JSONAPICache implements Cache { | |||
| this._capabilities.notifyChange(identifier, 'state', null); | |||
| } | |||
|
|
|||
| const fields = this._capabilities.schema.fields(identifier); | |||
There was a problem hiding this comment.
Technically I could only set this if one of the conditions below are met and we know we'll need it. Don't want to over-optimize
gitKrystan
commented
Feb 25, 2025
| @@ -1740,7 +1740,7 @@ class Model extends EmberObject implements MinimalLegacyRecord { | |||
| this.eachComputedProperty((name, meta) => { | |||
| if (isAttributeSchema(meta)) { | |||
| assert( | |||
| "You may not set `id` as an attribute on your model. Please remove any lines that look like: `id: attr('<type>')` from " + | |||
| "You may not set 'id' as an attribute on your model. Please remove any lines that look like: `id: attr('<type>')` from " + | |||
There was a problem hiding this comment.
Making this error message match the other one so the tests don't care about where specifically the error is thrown.
gitKrystan
commented
Feb 25, 2025
| test('_push does not require a modelName to resolve to a modelClass', function (assert) { | ||
| const store = this.owner.lookup('service:store'); | ||
| const originalCall = store.modelFor; | ||
| store.modelFor = function () { |
There was a problem hiding this comment.
Chris says we don't need this optimization anymore.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes a bug where we would notify changes to data that wasn't actually registered with the schema. This seems to only cause issues if the data is coincidentally tracked for some other reason (e.g.
idor a tracked property with the same name).NOTE:
Currently (including with this change), we do not discard this data in the cache. So when you get the resource data out of the cache, we include it. Since some APIs are dependent on payloads being complete when you send them back, some users may rely on the current behavior. So we don't delete the value itself so that it's still merged in. We should probably deprecate this eventually.
Notes for the release