-
-
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
[BUGFIX] Fix availability of properties in createRecord init #5413
Changes from all commits
22d481a
4b32222
64f2db1
1cbacd3
ec2f6df
0739672
74476c8
5fbbfab
684f10b
ba094ee
d578710
de55378
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,25 +24,13 @@ export default class Snapshot { | |
this._hasManyIds = Object.create(null); | ||
this._internalModel = internalModel; | ||
|
||
let record = internalModel.getRecord(); | ||
// TODO is there a way we can assign known attributes without | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make an issue so we can track fixing (or removing the TODO)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i left the skipped test for tracking :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue is #5419 |
||
// using `eachAttribute`? This forces us to lookup the model-class | ||
// but for findRecord / findAll these are empty and doing so at | ||
// this point in time is unnecessary. | ||
internalModel.eachAttribute((keyName) => this._attributes[keyName] = internalModel.getAttributeValue(keyName)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this broke So given: // app/models/foo.js
export default Model.extend({
bar: attr({ defaultValue: () => "baz" })
}); then let foo = store.createRecord('foo');
// this assertion breaks after upgrading to v3.2.0-beta.2 :pensive:
assert.deepEqual(foo.toJSON(), { bar: "baz" }); Currently the whole story around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems good to open an issue for this, I think it should be fixable... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be very fixable, surprised this broke though, re-examining my changes, will open an issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hrm, so I didn't pull the defaultValue logic into internalModel, nor am I sure we should. https://github.com/emberjs/data/pull/5413/files#diff-c2a90350f555423721228dfa13c137f2R130 That said, the proposal in #5419 would be a good direction to address this, as we can access each attribute via the record if it exists, and only copy lazily if it does not. |
||
|
||
/** | ||
The underlying record for this snapshot. Can be used to access methods and | ||
properties defined on the record. | ||
|
||
Example | ||
|
||
```javascript | ||
let json = snapshot.record.toJSON(); | ||
``` | ||
|
||
@property record | ||
@type {DS.Model} | ||
*/ | ||
this.record = record; | ||
record.eachAttribute((keyName) => this._attributes[keyName] = get(record, keyName)); | ||
|
||
/** | ||
/**O | ||
The id of the snapshot's underlying record | ||
|
||
Example | ||
|
@@ -73,7 +61,24 @@ export default class Snapshot { | |
*/ | ||
this.modelName = internalModel.modelName; | ||
|
||
this._changedAttributes = record.changedAttributes(); | ||
this._changedAttributes = internalModel.changedAttributes(); | ||
} | ||
|
||
/** | ||
The underlying record for this snapshot. Can be used to access methods and | ||
properties defined on the record. | ||
|
||
Example | ||
|
||
```javascript | ||
let json = snapshot.record.toJSON(); | ||
``` | ||
|
||
@property record | ||
@type {DS.Model} | ||
*/ | ||
get record() { | ||
return this._internalModel.getRecord(); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -339,34 +339,27 @@ Store = Service.extend({ | |
createRecord(modelName, inputProperties) { | ||
assert(`You need to pass a model name to the store's createRecord method`, isPresent(modelName)); | ||
assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string'); | ||
let normalizedModelName = normalizeModelName(modelName); | ||
let properties = copy(inputProperties) || Object.create(null); | ||
|
||
// If the passed properties do not include a primary key, | ||
// give the adapter an opportunity to generate one. Typically, | ||
// client-side ID generators will use something like uuid.js | ||
// to avoid conflicts. | ||
return this._backburner.join(() => { | ||
let normalizedModelName = normalizeModelName(modelName); | ||
let properties = copy(inputProperties) || Object.create(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just notating that we will eventially need to deal with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, we know. we use it a ton :/ |
||
|
||
if (isNone(properties.id)) { | ||
properties.id = this._generateId(normalizedModelName, properties); | ||
} | ||
// If the passed properties do not include a primary key, | ||
// give the adapter an opportunity to generate one. Typically, | ||
// client-side ID generators will use something like uuid.js | ||
// to avoid conflicts. | ||
|
||
// Coerce ID to a string | ||
properties.id = coerceId(properties.id); | ||
if (isNone(properties.id)) { | ||
properties.id = this._generateId(normalizedModelName, properties); | ||
} | ||
|
||
let internalModel = this._buildInternalModel(normalizedModelName, properties.id); | ||
internalModel.loadedData(); | ||
let record = internalModel.getRecord(); | ||
record.setProperties(properties); | ||
// Coerce ID to a string | ||
properties.id = coerceId(properties.id); | ||
|
||
// TODO @runspired this should also be coalesced into some form of internalModel.setState() | ||
internalModel.eachRelationship((key, descriptor) => { | ||
if (properties[key] !== undefined) { | ||
internalModel._relationships.get(key).setHasData(true); | ||
} | ||
let internalModel = this._buildInternalModel(normalizedModelName, properties.id); | ||
internalModel.loadedData(); | ||
return internalModel.getRecord(properties); | ||
}); | ||
|
||
return record; | ||
}, | ||
|
||
/** | ||
|
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.
IMHO, it would be nice to remove this (and rely on the auto-run). What do 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.
The problem is that too many of our APIs are sync, so we can't rely on
setTimeout
(or microtasks if backburner changes) for settledness at the moment. We need the work to complete by the time someone exits the ember-data world.This is something we would like to change, and a longterm goal of ours is to move more public APIs to being promise based to unlock optimizations around batching and updating without needing this sort of wonkiness.