chore: cleanup Implicit relationships #7463
Merged
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
|
Asset Size Report for 257e4cd IE11 Builds EmberData shrank by 229.0 B (-38.0 B compressed)WarningsThe uncompressed size of the package @ember-data/model has increased by 976.0 B. Changeset Full Asset Analysis (IE11)Modern Builds EmberData shrank by 229.0 B (-38.0 B compressed)WarningsThe uncompressed size of the package @ember-data/model has increased by 976.0 B. Changeset Full Asset Analysis (Modern)Modern Builds (No Rollup) EmberData shrank by 428.0 B (-67.0 B compressed)WarningsThe uncompressed size of the package @ember-data/record-data has increased by 99.0 B. Changeset Full Asset Analysis (Modern) |
igorT
reviewed
Apr 15, 2021
igorT
reviewed
Apr 16, 2021
| @@ -362,14 +375,21 @@ export default class Relationship { | |||
| if (!this._hasSupportForImplicitRelationships(recordData)) { | |||
| return; | |||
| } | |||
| let relationships = recordData._implicitRelationships; | |||
| let relationship = relationships[this.inverseKeyForImplicit]; | |||
| const relationships = implicitRelationshipsFor(recordData); | |||
There was a problem hiding this comment.
let if we are following our current style
igorT
reviewed
Apr 16, 2021
igorT
reviewed
Apr 16, 2021
| @@ -516,7 +538,7 @@ export default class Relationship { | |||
| const id = guidFor(inverseRecordData); | |||
|
|
|||
| if (this._hasSupportForImplicitRelationships(inverseRecordData) && seen[id] === undefined) { | |||
| const relationship = implicitRelationshipStateFor(inverseRecordData, this.inverseKeyForImplicit); | |||
| const relationship = implicitRelationshipStateFor(inverseRecordData, this.inverseKey); | |||
igorT
reviewed
Apr 16, 2021
igorT
reviewed
Apr 16, 2021
igorT
reviewed
Apr 16, 2021
| if (definition.inverseIsAsync !== undefined) { | ||
| return !!definition.inverseIsAsync; | ||
| } else { | ||
| if (((definition as unknown) as { inverseIsAsync?: boolean }).inverseIsAsync !== undefined) { |
There was a problem hiding this comment.
even though RelationshipDefinition implements RelationshipMeta typescript does not allow us to recast to it without first casting to unknown unless we change the return type of relationshipsDefinitionFor to be a union type. Since this is the only location that we leak our current implementation detail for meta on @ember-data/model it seemed better to leave the return type there alone.
igorT
reviewed
Apr 16, 2021
| kind: 'belongsTo' | 'hasMany'; | ||
| type: string; | ||
| key: string; | ||
| type: string; // related type |
40 tasks
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.
improves our typing throughout which allows us to drop some checks and extra initializations, makes where/when we are operating on an implicit relationship more clear (by using a flag instead of juggling keys). This allows us to have a single inverse key vs an inverseKey and inverseKeyForImplicit.
Only behavior change is that implicit keys are improved: before we were setting them to
undefined${this.key}at all times, which is the reason for the (single) test change.Motivation is to more closely align the current state with my next PR so that these somewhat more cosmetic changes are more easily distinguished from the logical changes.