chore: encapsulate InternalModel usage #6246
Conversation
runspired
force-pushed
the
cleanup/refactor-internal-model-usage
branch
from
59779f8
to
3796bd0
Compare
runspired
force-pushed
the
cleanup/refactor-internal-model-usage
branch
from
3796bd0
to
5cf43fd
Compare
|
The ember-m3 failure is because M3 reaches into the InternalModelMap. We could provide a shim to preserve this but once identifiers work enters the cleanup phase that map will go away entirely anyway. |
|
Opened an issue in M3 to track the failure there: ember-m3/ember-m3#301 It's a test-only failure. |
delete-merged-branch
bot
deleted the
cleanup/refactor-internal-model-usage
branch
| if (this.hasRecord) { | ||
| this._record.notifyBelongsToChange(key, record); | ||
| this._record.notifyBelongsToChange(key, this._record); |
There was a problem hiding this comment.
I don't think we need to pass in the second argument here, and in the case that we did, it would probably not be this._record but rather the value being set. Though that code path seems to have been removed, so we can just not pass the second arg
| let normalizedModelName = normalizeModelName(modelName); | ||
|
|
||
| let internalModel = this._internalModelForId(normalizedModelName, id); | ||
| const normalizedModelName = normalizeModelName(modelName); |
There was a problem hiding this comment.
I recall a discussion in which we decided to only use const for constants, and not inside methods.
| * | ||
| * @internal | ||
| */ | ||
| export default class InternalModelFactory { |
There was a problem hiding this comment.
Fwiw, it seems a bit odd to me that something named a factory would also keep a cache of things.
| * @internal | ||
| */ | ||
| _internalModelForId(modelName: string, id: string | null, lid: string | null): InternalModel { | ||
| return internalModelFactoryFor(this).lookup(modelName, id, lid); |
There was a problem hiding this comment.
If this is still the old clientId concept it might be misleading to call it lid
| internalModel.setId(id); | ||
| } | ||
|
|
||
| peekIdOnly(modelName: string, id: string): InternalModel | null { |
| return internalModel; | ||
| } | ||
|
|
||
| build(modelName: string, id: string | null, data?: any, clientId?: string | null) { |
There was a problem hiding this comment.
should we underscore the private methods like build?
| } | ||
| } | ||
|
|
||
| modelMapFor(modelName: string): InternalModelMap { |
There was a problem hiding this comment.
can we now refactor this to use this factory/map interface rather than having two things?
| internalModel.notifyErrorsChange(); | ||
| let internalModel = internalModelFactoryFor(this._store).peekId(modelName, id, clientId); | ||
|
|
||
| if (internalModel) { |
There was a problem hiding this comment.
We should probably assert if no IM here, that doesn't seem like a valid code path
| let internalModel = internalModelFactoryFor(this._store).peekId(modelName, id, clientId); | ||
|
|
||
| if (internalModel) { | ||
| internalModel.notifyPropertyChange(key); |
There was a problem hiding this comment.
Should we assert or warn here if no IM
| type: string; | ||
| clientId?: string; | ||
| id: string; | ||
| clientId?: string | null; |
There was a problem hiding this comment.
Can this be clientId?: string Or does that error?
Depends on #6239