-
-
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
Custom Model Classes #6313
Custom Model Classes #6313
Conversation
a74c055
to
89c5f63
Compare
353cfcd
to
4a4c310
Compare
type Primitive = string | number | boolean | null; | ||
interface Object { | ||
[member: string]: Value | undefined | ((...args: any[]) => any); | ||
} | ||
interface Arr extends Array<Object | Arr | undefined> {} | ||
|
||
type Value = Primitive | Object | Arr; | ||
*/ | ||
|
||
export type Record = Object; |
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.
rename to RecordInstance
in a new PR
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.
👍
*/ | ||
|
||
export default interface RecordDataRecordWrapper { | ||
rollbackAttributes(): 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.
once we have the proxy we need to ensure it is in the ts-upgrade-map
@@ -52,6 +51,14 @@ export default class RecordDataStoreWrapper implements IRecordDataStoreWrapper { | |||
_scheduleManyArrayUpdate(modelName: string, id: string | null, clientId: string, key: string): void; | |||
_scheduleManyArrayUpdate(modelName: string, id: string, clientId: string | null | undefined, key: string): void; | |||
_scheduleManyArrayUpdate(modelName: string, id: string | null, clientId: string | null | undefined, key: string) { | |||
if (!hasValidId(id, clientId)) { |
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 think this goes away if we land #6289
packages/store/addon/-private/system/store/record-data-store-wrapper.ts
Outdated
Show resolved
Hide resolved
packages/store/addon/-private/system/schema-definition-service.ts
Outdated
Show resolved
Hide resolved
this._id = id; | ||
} | ||
|
||
get fields(): Map<string, 'attribute' | 'belongsTo' | 'hasMany'> { |
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 should fast follow a deprecation, I believe as part of the RFC we were looking at the shim being a place to do. Seems good to have full API support at the beginning though.
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.
👍
get relationshipsByName(): Map<string, RelationshipSchema> { | ||
let relationships = this.store._relationshipsDefinitionFor(this.modelName, this._id); | ||
//TODO Talk to mnorth about needing a cast | ||
return new Map(Object.entries(relationships) as [string, RelationshipSchema][]); |
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 worried about these being uncached getters?
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 can't cache we should document why we can't assume cacheable and how users should approach using these APIs when they are cacheable.
@@ -2737,21 +2840,36 @@ class Store extends Service { | |||
// contains unknown attributes or relationships, log a warning. | |||
|
|||
if (ENV.DS_WARN_ON_UNKNOWN_KEYS) { | |||
let modelClass = this.modelFor(modelName); | |||
let unknownAttributes, unknownRelationships; |
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 will only work for the case where the schemas for all type
are identical, not the one where they may differ by identifier.
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 is a low priority fix)
} | ||
} | ||
|
||
_attributesDefinitionFor(modelName) { |
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.
double-check the identifiers
codepath for retrieving attributes does not pull from this info
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.
(same for relationships definition)
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.
Other than explicit notes/comments, TS LGTM
packages/store/addon/-private/system/relationships/state/relationship.ts
Outdated
Show resolved
Hide resolved
packages/store/addon/-private/system/relationships/state/relationship.ts
Show resolved
Hide resolved
packages/store/addon/-private/system/store/record-data-store-wrapper.ts
Outdated
Show resolved
Hide resolved
|
||
export interface SchemaDefinitionService { | ||
doesTypeExist(modelName: string): boolean; | ||
attributesDefinitionFor(identifier: RecordIdentifier | string): AttributesSchema; |
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.
minor nit, overloads would allow separate variable name and documentation for the modelName case
const Tag = DS.Model.extend({ | ||
name: DS.attr('string'), | ||
people: DS.hasMany('person'), | ||
peopleDidChange: observer('people', function() { |
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 should be very careful with this one, possibly confirm with some apps powering their selects with hasMany's that this doesn't break them.
packages/-ember-data/tests/unit/custom-class-support/custom-class-model-test.ts
Outdated
Show resolved
Hide resolved
2d48433
to
f720741
Compare
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.
Overall 👍 assuming comments are all resolved
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 need to ensure that CI is green before landing this.
ce76ce0
to
327ac50
Compare
@rwjblue CI is green |
travis allow failures allo fail chaneg tracker
9d3a11e
to
da004ca
Compare
Implements RFC emberjs/rfcs#487