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

Reduce dependency on ArrayComputed and ReduceComputed #10582

Closed
wants to merge 2 commits into from

Conversation

wagenet
Copy link
Member

@wagenet wagenet commented Mar 6, 2015

DO NOT MERGE YET

This removes the dependency of the Ember.computed array macros on ArrayComputed and ReduceComputed. The new path is currently slower but significantly less complex. Once Glimmer is merged (#10501) the slowness introduced by this PR is unlikely to cause problems.

This PR also removes all internal dependencies on ArrayComputed and ReduceComputed allowing these methods to be removed entirely from core. (If desired, we can make a plugin for them.)

KNOWN DIFFERENCES/ISSUES

  • Not everything returns an Ember.A anymore.
  • Resulting arrays are no longer modified in place
  • Ordering of some methods like union and intersect may be different than before. I think this is acceptable since sorting isn't part of the API contract for these.
  • Property sorting code probably causes more churn than the previous version. I think we should deprecate this behavior.

TODO

  • Add deprecation warnings
  • Consider moving the reduce shim to EnumerableUtils and/or an array shim

@wagenet wagenet changed the title Start remove ArrayComputed Start removing ArrayComputed Mar 6, 2015
@wagenet wagenet added the Cleanup label Mar 6, 2015
@stefanpenner
Copy link
Member

The great culling begins

@fivetanley
Copy link
Member

Resulting arrays are no longer modified in place

YESSSSSS

@wagenet
Copy link
Member Author

wagenet commented Mar 6, 2015

@fivetanley was the in place modification an issue for you? I was thinking of that as a slight loss (though worth it because of reduced complexity).

@wagenet wagenet changed the title Start removing ArrayComputed Reduce dependency on ArrayComputed and ReduceComputed Mar 6, 2015
import compare from 'ember-runtime/compare';

var a_slice = [].slice;

// TODO: Consider putting this with other array shims
Copy link
Contributor

Choose a reason for hiding this comment

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

In 2.0.0 it seems like there might be an opportunity to take compat ES5 stuff into an 'ember.compat.js'. This way developers can shed even more weight if they only support evergreen browsers. It also draws a clean line for deprecations when ember decides not to support non ES5 browsers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I definitely think there is some improvement we can do here. I don't want to get too caught up on that stuff in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. :shipit:

@fivetanley
Copy link
Member

@fivetanley was the in place modification an issue for you?

In theory? Absolutely not, it's a great idea. In practice? It was the source of many zalgo bugs and random test failures. I know @kategengler ran into this a lot too.

@kategengler
Copy link
Member

❤️ I'm very excited for this, I've been avoiding any of the computed macros that used arrayComputed and reduceComputed because of running into many of the bugs around them.

@rwjblue
Copy link
Member

rwjblue commented May 25, 2015

Now that the glimmer work has landed, this is ready to be considered.

@wagenet - Can you rebase?

@stefanpenner
Copy link
Member

cc @hjdivad

@Leooo
Copy link
Contributor

Leooo commented May 27, 2015

Hello, I would like to be confirmed if arrayComputed / reduceComputed features will still be present in future Ember versions, at least as compatible modules: I'm into a complex App where this feature seems essential to not overwhelm the UI. Are there really irremediable bugs in arrayComputed / reduceComputed, or is this mostly due to an incomplete understanding of such complex methods from some in the Ember community?

As I'm quite new to Ember I may miss some things, are there any other solutions which would allow to do complex operations on a huge array and not re-calculate everything when one of its element's properties changes?

Thanks.

@stefanpenner
Copy link
Member

superseded by: #11513

@stefanpenner
Copy link
Member

Hello, I would like to be confirmed if arrayComputed / reduceComputed features will still be present in future Ember versions, at least as compatible modules: I'm into a complex App where this feature seems essential to not overwhelm the UI. Are there really irremediable bugs in arrayComputed / reduceComputed, or is this mostly due to an incomplete understanding of such complex methods from some in the Ember community?

The bugs are quite tricky, luckily the new glimmer engine can handle re-renders of total array changes quite quickly. Obvious extremely large sets of data may have some issues with our new approach. For those i suspect a better experience would be using something like http://square.github.io/crossfilter/. I am sure there are other solutions.

Once this lands, I'll put some cross filter examples together. I suspect it would make a great ember example :)

@Kilowhisky
Copy link

Did these examples happen to get made? I'm facing performance issues due to these changes and am looking for an alternative. In my case i have arrays of 1000 items that are constantly being added to in a real time scenario.

@stefanpenner
Copy link
Member

I don't believe so

@Leooo
Copy link
Contributor

Leooo commented Nov 3, 2015

crossfilter seems to be a good solution. I don't realize if https://github.com/Wildhoney/EmberCrossfilter can be adapted easily to work with Ember 1.12.

@stefanpenner
Copy link
Member

crossfilter seems to be a good solution. I don't realize if https://github.com/Wildhoney/EmberCrossfilter can be adapted easily to work with Ember 1.12.

:)

@Leooo
Copy link
Contributor

Leooo commented Nov 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants