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

remove needless change events when creating a recordArrays #4947

Merged
merged 10 commits into from
May 16, 2017

Conversation

stefanpenner
Copy link
Member

@stefanpenner stefanpenner commented Apr 26, 2017

When possible we should create new record arrays with there content, rather then creating them, and later adding there content. This prevents un-needed change events from firing. Some Older API's make this a tad tricky, but modern usages should not be penalized.

  • liveRecordArrays
  • adapterPopulatedRecordArrays

…ent (rather then add them after, and broadcasting change events)
@stefanpenner stefanpenner changed the title remove needless change events when creating a liveRecordArray remove needless change events when creating a recordArrays Apr 26, 2017
let modelsToAdd = [];
let modelsToRemove = [];

for (let i = 0; i < internalModels.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does our new diffArray method work here or is the data structure different?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. I didn't think of that, I should try

@@ -161,6 +161,75 @@ test('recordArray.replace() throws error', function(assert) {
}, Error('The result of a server query (on person) is immutable.'), 'throws error');
});

test('only pass record array if explicitly in arity', function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

as written, this test name is hard to understand.

maybe 'pass record array to adapter.query based on arity'

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update

});
});

test('only pass record array if explicitly in arity', function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

as written, this test name is hard to understand.

maybe 'pass internalModels to createStore based on arity'

@stefanpenner
Copy link
Member Author

fun fun

Error: The addon `ember-cli-htmlbars` requires the npm package `ember-cli` to be above 0.1.2, but you have undefined.

@rwjblue
Copy link
Member

rwjblue commented May 10, 2017

Hmm. Can you double check and confirm we have ember-cli-htmlbars@1.3.2? I released a bad patch release earlier (1.3.1)...

@stefanpenner
Copy link
Member Author

stefanpenner commented May 10, 2017

@rwjblue the issue did start with 1.3.2 but it appears to not be the fault of 1.3.2. Something either in the dep checker/ember-cli or blueprint-test helpers is causing the grief.

I got a few other things to do this morning, then I'll debug further.

@stefanpenner
Copy link
Member Author

@rwjblue TL;DR

These tests https://github.com/emberjs/data/blob/master/node-tests/blueprints/transform-test.js#L45 create a project at tmp/<something>/my-app that have a node_modules that contains only ember-cli-mocha, but the package.json contains ember-cli-htmlbars. Due to the node resolution algorithm ember-cli-htmlbars is being discovered in the node_module directory of ember-data itself.

I believe modifyPackages may not be working correctly..

@stefanpenner stefanpenner force-pushed the remove-change-removes branch 5 times, most recently from 258f158 to dd08392 Compare May 15, 2017 20:39
@stefanpenner stefanpenner merged commit 58bd396 into master May 16, 2017
@stefanpenner stefanpenner deleted the remove-change-removes branch May 16, 2017 05:46
@stefanpenner
Copy link
Member Author

finally green, although appveyor test are still hanging (unrelated to this PR, but I think we know whatsup in ember-cli land causing this...)

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.

None yet

4 participants