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

Ember.Array methods return Array rather than Ember.Array #12003

Closed
indirect opened this issue Aug 6, 2015 · 7 comments
Closed

Ember.Array methods return Array rather than Ember.Array #12003

indirect opened this issue Aug 6, 2015 · 7 comments

Comments

@indirect
Copy link

indirect commented Aug 6, 2015

Ember.Array methods that return an array, like filterBy, return a bare JS Array rather than an Ember.Array. This makes chaining Ember.Array methods impossible with prototype extensions disabled. Here's an example:

// prototype extensions on
Ember.A(someArray).filterBy(‘property’).mapBy('otherProperty') // => [1, 2, 3]

// prototype extensions off
Ember.A(someArray).filterBy('property').mapBy('otherProperty') // => undefined is not a function

Ember.A(Ember.A(someArray).filterBy('property')).mapBy('otherProperty') // => [1, 2, 3]

I can't find any discussion of this in the documentation, so I'm opening an issue. It seems like it would make sense for Ember.Array methods that return an array to return an Ember.Array, so that chaining Ember.Array methods becomes possible.

@stefanpenner
Copy link
Member

Until we can subclass natives, there isn't a reasonable & performant way to do this.

There are two potential ideas:

  • use lodash – Packager ember-cli/ember-cli#4211 will unlock efficient tree-shaking builds of lodash
  • transition embers helpers to follow an adapter similar to lodash pattern, i suspect just using lodash may be the better option.

I believe that making this work for Ember.A( is a WONTFIX issue.

please note. Ember.A isn't meant for chainable helpers, rather a quirk of todays internals that require prototype extensions for observability. It is a side-affect that filterBy and friends are available via this method.

I began working to alleviate this internal dependency, making Ember.A redundant, just haven't had a just to bring this to completion.

@indirect
Copy link
Author

indirect commented Aug 6, 2015

Okay. So I should be using Ember.Enumerable.filterBy.apply(array, function(){}) when I want to use filterBy with prototype extensions turned off? If that's true, I can see how lodash seems like the better option. :)

Is there a way to get the property-based ease of filterBy('propName', 'wantedValue') via lodash? Or am I just out of luck?

@stefanpenner
Copy link
Member

Is there a way to get the property-based ease of filterBy('propName', 'wantedValue') via lodash? Or am I just out of luck?

Unsure, i do know some of the mappers support deep properties in lodash.

So I should be using Ember.Enumerable.filterBy.apply(array, function(){}) when I want to use filterBy with prototype extensions turned off?

If there is a missing feature, i think this might make good addon territory for now. A set of helpers similar to the following might be interesting.

function filterBy(array, ...args) {
  return Ember.Enumerable.filterBy.apply(array, args)
}

@indirect indirect closed this as completed Aug 6, 2015
@sukima
Copy link
Contributor

sukima commented Aug 26, 2015

Ember.Enumerable.filterBy does not exists. Ember.Enumerable.mixins[0].properties.filterBy does however. How did this work 20 days ago?

sukima added a commit to sukima/ember-cli-select-picker that referenced this issue Aug 26, 2015
When prototype extensions are disabled as they are in an Ember addon you
lose all ability to chain the Ember enumerable functions. This adds a
set of helper functions that will apply the Ember enumerable functions
when needed.

For more information on this see this discussion:
emberjs/ember.js#12003
sukima added a commit to sukima/ember-cli-select-picker that referenced this issue Aug 26, 2015
When prototype extensions are disabled as they are in an Ember addon you
lose all ability to chain the Ember enumerable functions. This adds a
set of helper functions that will apply the Ember enumerable functions
when needed.

For more information on this see this discussion:
emberjs/ember.js#12003
@sukima
Copy link
Contributor

sukima commented Aug 26, 2015

@stefanpenner, You said:

use lodash – ember-cli/ember-cli#4211 will unlock efficient tree-shaking builds of lodash

Does this mean Ember will have a version of lodash included?

I think that perhaps there is a context confusion here. I ran in to the problem of chaining and using Ember.A() everywhere because I am maintaining an ember-cli addon which by design has prototype extensions turned off. This means that if an addon does any significant amount of data manipulation then they have to do one of:

  1. Perform a large bit of heavy lifting to deep dive into the Enumerable mixin.
  2. Or force the inclusion of a third party library like lodash.

The first one seems like a disservice to ember addon authors and the second seems like an insidious expectation on the consumers of such an addon. Advice?

Also here is how I managed to handle array manipulation in my ember addon. The down side is I could not find a way to accomplish this with out Ember.A() which as you described is redundent and probubly slated to go away! 😰

// Ember Addons need to be coded as if Ember.EXTEND_PROTOTYPES = false
// Because of this we need to make our own proxy functions to apply as one offs
// to native arrays.
const emberArrayFunc = function(method) {
  return function(ctx, ...args) {
    // We have to look this up in the context of the returned function. The properties
    // are not initialized (available) when evaluating the ES6 module. They are only
    //  available at the time the helper functions are called.
    const props = Ember.Enumerable.mixins[0].properties; // Wow this was deep in there
    // Ember.Enumerable.____ does not exist, 'cause it is a mixin.
    Ember.assert(
      `Ember.Enumerable has no method ${method}`,
      Ember.typeOf(props[method]) === 'function'
    );
    const result = props[method].apply(Ember.A(ctx), args);
    if (Ember.typeOf(result) === 'array') {
      return Ember.A(result);
    } else {
      return result;
    }
  };
};
const _contains = emberArrayFunc('contains');
const _mapBy    = emberArrayFunc('mapBy');
const _filterBy = emberArrayFunc('filterBy');
const _findBy   = emberArrayFunc('findBy');
const _uniq     = emberArrayFunc('uniq');
const _compact  = emberArrayFunc('compact');

@stefanpenner
Copy link
Member

Does this mean Ember will have a version of lodash included?

no, it means using lodash will be pay as you go as that PR explains.

@stefanpenner
Copy link
Member

the final solution is two fold.

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

No branches or pull requests

3 participants