-
-
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
[CHORE]: Extract internalModel access to identifiers for Relationships #7262
Conversation
@@ -1580,7 +1580,7 @@ abstract class CoreStore extends Service { | |||
typeof modelName === 'string' | |||
); | |||
|
|||
const resource = constructResource(modelName, ensureStringId(id)); | |||
const resource: ResourceIdentifierObject = constructResource(modelName, ensureStringId(id)); |
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.
Do we like to type the return value? If no value, I can remove
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.
If it's not super clear, it's 👍
} else { | ||
this.parent = parentIMOrIdentifier.recordReference; | ||
this.parentInternalModel = parentIMOrIdentifier; | ||
} |
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.
If the goal is to remove references to the internal model property as well, I can make this this.parentIdentifier
and lookup by identifier in each corresponding spot that needs an internal model. Let me know what you think!
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.
If we have a path of fully getting away from internal model, we probably want to bundle this up together, landing just the lookup part might be bad for perf in hot paths
@@ -31,6 +34,10 @@ function isResourceIdentiferWithRelatedLinks( | |||
|
|||
export const INTERNAL_MODELS = new WeakMap<Reference, InternalModel>(); |
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.
Sort of along the same lines as the other question - The API changed for creating References from InternalModel to identifiers. However, do we want to also change our caching layer as well with this refactor? Or does this seem right to keep as is (various places in belongs-to/has-many reference IM specific properties)?
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.
Replacing the caching layer is 👍
Asset Size Report for 84ea514 IE11 Builds The size of the library EmberData has increased by 132.0 B (94.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/store has increased by 132.0 B. Changeset
Full Asset Analysis (IE11)
Modern Builds The size of the library EmberData has increased by 132.0 B (94.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/store has increased by 132.0 B. Changeset
Full Asset Analysis (Modern)
Modern Builds (No Rollup) The size of the library EmberData has increased by 509.0 B (132.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/store has increased by 509.0 B. Changeset
Full Asset Analysis (Modern)
|
Performance Report for 84ea514 Relationship Analysis
|
INTERNAL_MODELS.set(this, internalModel); | ||
} | ||
} else { | ||
INTERNAL_MODELS.set(this, identifierOrInternalModel as InternalModel); |
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.
Are we ok coercing variables to a specific 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.
Yeah, usually a type guard would be great, but we don't have one in this case.
@@ -33,7 +33,6 @@ export interface ExistingResourceIdentifierObject { | |||
type: string; | |||
|
|||
/** |
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.
This seems like an accident
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.
I thought lid
was a part of the JSONAPI spec. But it looks like it is upcoming.
8021c5a
to
a6850fb
Compare
84c4f38
to
420db2a
Compare
af82efe
to
5de23dc
Compare
f3f23f3
to
140b1d8
Compare
@@ -144,8 +144,8 @@ module('integration/references/belongs-to', function(hooks) { | |||
var personReference = store.getReference('person', 1); | |||
var familyReference = person.belongsTo('family'); | |||
|
|||
assert.ok(personReference); | |||
assert.equal(familyReference.parent, personReference); | |||
assert.ok(personReference, 'person reference is present'); |
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.
❤️
@@ -1436,9 +1439,20 @@ abstract class CoreStore extends Service { | |||
} | |||
const type = normalizeModelName(modelName); | |||
const normalizedId = ensureStringId(id); | |||
const resource = constructResource(type, normalizedId); | |||
const resource: ResourceIdentifierObject = constructResource(type, normalizedId); |
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.
this type/id should be a recordIdentifer not a resource
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.
Sounds like we need to adjust the return type of constructResource
to ResourceIdentifierObject | RecordIdentifier
?
const resource: ResourceIdentifierObject = constructResource(type, normalizedId); | ||
|
||
if (RECORD_ARRAY_MANAGER_IDENTIFIERS) { | ||
const identifier: StableRecordIdentifier = identifierCacheFor(this).getOrCreateRecordIdentifier(resource); |
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 shouldn't getOrCreateRecordIdentifier
here
@@ -82,7 +88,7 @@ export default class BelongsToReference extends Reference { | |||
} | |||
|
|||
_resource() { | |||
return INTERNAL_MODELS.get(this)?._recordData.getBelongsTo(this.key); | |||
return internalModelForReference(this)?._recordData.getBelongsTo(this.key); |
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.
You should be able to get the record data if you have the identifier right?
let resource = this._resource(); | ||
if (resource && resource.data) { | ||
let inverseInternalModel = store._internalModelForResource(resource.data); | ||
let inverseInternalModel = this.store._internalModelForResource(resource.data); |
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.
👍
|
||
// TODO inverse | ||
} | ||
|
||
_resource() { | ||
return INTERNAL_MODELS.get(this)?._recordData.getHasMany(this.key); | ||
return internalModelForReference(this)?._recordData.getHasMany(this.key); |
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.
Should be able to get recordData via reference
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.
Are you suggesting we make a public getter for recordData
on the Record instance?
If not, I was assuming here that the only way to get recordData
was through the InternalModel. Perhaps there is a better way?
@@ -29,7 +32,15 @@ function isResourceIdentiferWithRelatedLinks( | |||
return value && value.links && value.links.related; | |||
} | |||
|
|||
export const INTERNAL_MODELS = new WeakMap<Reference, InternalModel>(); | |||
export const REFERENCE_CACHE = new WeakMap<Reference, InternalModel | StableRecordIdentifier>(); |
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.
Once RECORD_ARRAY_MANAGER_IDENTIFIERS are turned on, lets make sure to simplify this, maybe leave a todo comment and rename to be more descriptive
|
||
export function internalModelForReference(reference: Reference): InternalModel | null | undefined { | ||
if (RECORD_ARRAY_MANAGER_IDENTIFIERS) { | ||
return internalModelFactoryFor(reference.store).peek(REFERENCE_CACHE.get(reference) as StableRecordIdentifier); |
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.
I am fine with the messy TS as we are turning this FF very soon
INTERNAL_MODELS.set(this, internalModel); | ||
constructor(public store: CoreStore, identifierOrInternalModel: InternalModel | StableRecordIdentifier) { | ||
if (RECORD_ARRAY_MANAGER_IDENTIFIERS) { | ||
REFERENCE_CACHE.set(this, identifierOrInternalModel as StableRecordIdentifier); |
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.
I don't think this needs a cast
04d8bb0
to
9af131f
Compare
return null; | ||
} else { | ||
return internalModelForReference(this)!.id; | ||
} |
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.
From meeting - an identifier
getter probably makes sense. Will discuss in next week's meeting.
9af131f
to
88e9205
Compare
@@ -61,6 +61,10 @@ abstract class Reference { | |||
} | |||
} | |||
|
|||
get recordData() { | |||
return internalModelForReference(this)?._recordData; |
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.
Can you get the identifier here as we for when we do have it and use it to lookup the record data
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.
Done!
94209bf
to
a1fe59c
Compare
@@ -16,6 +16,6 @@ import { dasherize } from '@ember/string'; | |||
@param {String} modelName | |||
@return {String} normalizedModelName | |||
*/ | |||
export default function normalizeModelName(modelName) { | |||
export default function normalizeModelName(modelName: string): string { |
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.
❤️
This reverts commit 840d265.
…fier might be the argument
a1fe59c
to
84ea514
Compare
@snewcomer Have you thought about registering for Github Sponsorships? 😄 |
ref #6166
past work #6715