-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[BUGFIX identifiers] Address issue with polymorphic findRecord #7363
Conversation
Performance Report for 1bc07ea Relationship Analysis
|
Asset Size Report for 1bc07ea IE11 Builds EmberData increased by 111.0 B uncompressed but decreased by 1.0 B compressedWarningsThe uncompressed size of the package @ember-data/store has increased by 111.0 B. Changeset
Full Asset Analysis (IE11)
Modern Builds EmberData increased by 111.0 B uncompressed but decreased by 1.0 B compressedWarningsThe uncompressed size of the package @ember-data/store has increased by 111.0 B. Changeset
Full Asset Analysis (Modern)
Modern Builds (No Rollup) The size of the library EmberData has increased by 123.0 B (66.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/store has increased by 123.0 B. Changeset
Full Asset Analysis (Modern)
|
@@ -343,6 +343,15 @@ export class IdentifierCache { | |||
let newId = coerceId(data.id); | |||
let existingIdentifier = detectMerge(this._cache.types, identifier, data, newId, this._cache.lids); | |||
|
|||
if (!existingIdentifier) { | |||
if (identifier.type !== data.type && data.type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's if (existingIdentifier)
statement below, maybe it would be reasonable to have this as else if
after that?
I'd also prefer to add a comment about what sort of case this is meant to deal with, but totally up to you folks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We still want the other statement to run, in fact the whole point of doing this here is to get line 357
identifier = this._mergeRecordIdentifiers(keyOptions, identifier, existingIdentifier, data, newId as string);
to perform the merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Great point re: comment, adding that and merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG makes total sense. Thank you
Hey @snewcomer @igorT, is there anything I can do to help merge this? |
Add test for cache Alternaive solution to 7325
* [BUGFIX identifiers] Address issue with polymorphic findRecord Fixes issue when encountering polymorphic records with a subtype on initial load. If the returned record did not share the asked for type, identifiers cache would get into an incoherent statet. Co-authored-by: Dmitry Krasnoukhov <dmitry@krasnoukhov.com>
* [BUGFIX identifiers] Address issue with polymorphic findRecord Fixes issue when encountering polymorphic records with a subtype on initial load. If the returned record did not share the asked for type, identifiers cache would get into an incoherent statet. Co-authored-by: Dmitry Krasnoukhov <dmitry@krasnoukhov.com>
I see that you've merged this into master and into 3.23, but do you think this is something that can also go into 3.20 LTS? |
Fixes issue when encountering polymorphic records with a subtype on initial load.
Builds on #7325