-
-
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
[FEAT identifiers] Implements RFC #403 #5914
Conversation
a37bf3e
to
f386769
Compare
* [FEAT identifiers] adds uuid-v4 util * [FEAT identifiers] adds ts-interface for json-api concepts w/ember-data flavor * [FEAT identifier] adds toString to interface for even more debuggability * [FEAT identifier] makes debug members of the interface optional * [FEAT IdentifierCache] implements the cache to spec * record-data-wrapper -> store-wrapper.js (fix name) * fix store usage by relationships without leaking to RecordData
1390073
to
0bcf4e1
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.
Spotted this stuff on a quick pass. We should review this together in more depth sometime soon
@@ -0,0 +1 @@ | |||
export type Dict<K extends string, V> = { [KK in K]: V }; |
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 not a type-safe interface, since the possibility of missing values isn't accounted for
export type Dict<K extends string, V> = { [KK in K]: V }; | |
export type Dict<K extends string, V> = { [KK in K]: V | undefined }; |
|
||
constructor(store, identifierCache) { | ||
this._identifierCache = identifierCache; | ||
this._store = store; |
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.
These two can be converted to parameter properties
constructor(private _store: any, private _identifierCache: IdentifierCache) {}
private _cache = { | ||
lids: Object.create(null) as IdentifierMap, | ||
types: Object.create(null) as TypeMap | ||
}; |
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 may want to favor use of a type annotation over casting
this._generate = configuredGenerationMethod || defaultGenerationMethod; | ||
this._update = configuredUpdateMethod || defaultUpdateMethod; | ||
this._forget = configuredForgetMethod || defaultForgetMethod; | ||
this._reset = configuredResetMethod || defaultResetMethod; |
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.
Is there a reason why class field initializers aren't used here?
|
||
function isNonEmptyString(str?: string | null): str is string { | ||
return typeof str === 'string' && str.length > 0; | ||
} |
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 can make this a more useful check through using a generic.
function isNonEmptyString<T extends string>(str: T | undefined | null | ''): str is T {
return typeof str === 'string' && str.length > 0;
}
This will allow it to work correctly with unioned string literal types
let x = 'bar' as '' | 'foo' | 'bar';
if (isNonEmptyString(x)) {
x // 'foo' | 'bar' (yours would have included '')
} else {
x // '' (yours would have been never)
}
lid: Object.create(null), | ||
id: Object.create(null), | ||
_allIdentifiers: [], | ||
} as KeyOptions; |
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.
prefer type annotation over casting
Replaced by #6247 |
Identifiers RFC