Skip to content
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

Cleanup record array manager #4597

Merged
merged 5 commits into from
Oct 22, 2016
Merged

Conversation

stefanpenner
Copy link
Member

@stefanpenner stefanpenner commented Oct 19, 2016

Related to improving duplicate work:

  • avoid addObject in recordArrays instead use pushObject, as the record-array-manager already ensures only pushing if needed, by first consulting internalModel._recordArrays
  • ensure we remove adapterPopulatedRecordArrays when removing an internalModel from them
  • rename loadRecords to a more private and correctly named _setInternalModels
  • backfill many tests
  • split unit/integration tests up based on if they are unit or integration

@stefanpenner stefanpenner force-pushed the cleanup-record-array-manager branch 5 times, most recently from f3228b7 to db29f12 Compare October 21, 2016 20:18

test('stores the metadata off the payload', function(assert) {
let recordArray = store.recordArrayManager
.createAdapterPopulatedRecordArray(store.modelFor('person'), null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can re-use Person like we do 2 tests below.


assert.throws(() => {
recordArray.replace();
}, Error('The result of a server query (on (subclass of DS.Model)) is immutable.'), 'throws error');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're overwriting toString() in the Person above. Not sure why it's used here though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the assertion to be 'The result of a server query (on Person) is immutable.'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened issue: #4615

function assertRecordHasRecordArray(assert, record, recordArray) {
if (!assert) { throw TypeError('assertRecordHasRecordArray first argument must be QUnit\'s assert'); }
if (!record) { throw TypeError('assertRecordHasRecordArray second argument must be a record'); }
if (!assert) { throw TypeError('assertRecordHasRecordArray third argument must be a recordArray'); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/assert/recordArray/

function assertRecordDoesNOTRecordArray(assert, record, recordArray) {
if (!assert) { throw TypeError('assertRecordHasRecordArray first argument must be QUnit\'s assert'); }
if (!record) { throw TypeError('assertRecordHasRecordArray second argument must be a record'); }
if (!assert) { throw TypeError('assertRecordHasRecordArray third argument must be a recordArray'); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhmm lecker, copy pasta 🍜

@@ -73,15 +76,19 @@ export default RecordArray.extend({
*/
loadRecords(internalModels, payload) {
let token = heimdall.start('AdapterPopulatedRecordArray.loadRecords');

// TODO: initial load should not cause change events at all, only
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation?

let token = heimdall.start('AdapterPopulatedRecordArray.loadRecords');
_setInternalModels(internalModels, payload) {
let token = heimdall.start('AdapterPopulatedRecordArray._setInternalModels');
let operations = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be a null object?

* make adding/removing from recordArrays private
* make recordArray add/remove support batches
* rename addInternalModel -> _pushInternalModels to be more accurate (push not add, and plural))
* [PERF] don’t use addObject in recordArrays, as the RecordArrayManager via internalRecord._recordArrays already checks containment, no need in duplicating that work by re-scanning addObject for each record added to the recordArray.
* add tests, covering the duplication behavior
* add comments, to ensure we don’t accidentally go back to pushObject
* cleanup code
* move non-unit tests to integration folder
…uring loadRecords

Replace the values contained within content, rather then replacing content. Without this we:

* signal `content` changed (no point)
* signal strange array mutations on the ArrayProxy (from empty content, instead of previous content);

Backfill missing tests
* make the very private method more private \w underscore
* rename loadRecords to _setInternalModels
@stefanpenner
Copy link
Member Author

updated re: @igorT feedback

@stefanpenner stefanpenner merged commit da3f9a6 into master Oct 22, 2016
@stefanpenner stefanpenner deleted the cleanup-record-array-manager branch October 22, 2016 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants