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

Accept an array as valuePath #456

Open
buschtoens opened this issue Jul 13, 2017 · 7 comments
Open

Accept an array as valuePath #456

buschtoens opened this issue Jul 13, 2017 · 7 comments

Comments

@buschtoens
Copy link
Collaborator

I often find myself needing to compute a cell's value dependent of more than one field of the row. But the current API allows only passing one key as the valuePath.

I work around this by just accessing whatever properties I need from the row attribute that is passed to the cellComponent. I think that's pretty ugly.

If we would allow users to pass a string or an array of strings to valuePath, then (in the latter case) we could pass an array of values as value to the cellComponent.

@buschtoens
Copy link
Collaborator Author

I gave this some thought and while I still think that this is a worthwhile feature, implementing it is a bit more complex than I initially expected. There are two solutions I could think of.

Class Computed Property

Using createClassComputed we could probably create a computed property that returns the correct value or array of values depending on valuePath.

Template Helper

Ideally, I would like to use a hypothetical {{get-multiple}} helper (open for name suggestions).

const sourceObject = {
  foo: 'ember',
  bar: 'light',
  qux: {
    quax: 'table'
  }
};
const paths = ['bar', 'qux.quax'];
{{get-multiple sourceObject paths}} {{! => ['light', 'table'] }}

The helper is complex and reusable enough, that it's justified to be released as its own addon. The code should be based on ember-glimmer/helpers/get.

@alexander-alvarez
Copy link
Collaborator

@buschtoens
Copy link
Collaborator Author

Exactly. And I think collect is a better fitting name. Thanks!

Unfortunately we won't be able to use the collect CP, as even for class based helpers the value has to be returned by compute() and invalidation is triggered by calling recompute(). 🙁

@buschtoens
Copy link
Collaborator Author

buschtoens commented Jul 19, 2017

I created an ember-collect-helper addon. Unfortunately I wasn't able to implement it as a "Glimmer helper" since not all required packages are published to npm / accessible in the context of an app or addon.

I'm gonna submit a PR with a PoC soon. I'm interested in how much faster {{get}} is compared to {{collect}} in the base case. If the regression is significant, I'd say we put this enhancement on hold until {{collect}} can be implemented as a "Glimmer helper".

buschtoens added a commit to buschtoens/ember-light-table that referenced this issue Jul 19, 2017
@buschtoens
Copy link
Collaborator Author

buschtoens commented Jul 20, 2017

I've had a good night's sleep about it and got a viable idea for a third option.

Create a new cell type multi-value

In order to not penalize all users with a theoretically possible performance hit (that I wasn't able to measure tough), we can make this feature opt-in by using a different cell type. This also means that we can factor this out into an extra addon.

Something similar to this should do the trick.

import { get, set } from '@ember/object';
import { collect, reads } from '@ember/object/computed';
import { isArray } from '@ember/array';
import BaseCell from 'ember-light-table/components/cells/base';

export default class extends BaseCell {
  didReceiveAttrs() {
    super.didReceiveAttrs(...arguments);

    const valuePath = get(this, 'column.valuePath');
    if (isArray(valuePath)) {
      set(this, 'rawValue', collect(...valuePath));
    } else {
      set(this, 'rawValue', reads(valuePath));
    }
  }
}

Also TIL: setting a computed property at runtime works just fine. 😁
Edit: ...once.

image

@buschtoens
Copy link
Collaborator Author

Turns out creating computed properties at run time isn't as easy as it seems.

import Ember from 'ember';
import { defineProperty, get } from '@ember/object';
import { collect, reads } from '@ember/object/computed';
import { isArray } from '@ember/array';
import BaseCell from 'ember-light-table/components/cells/base';

const { propertyDidChange, propertyWillChange } = Ember;

function forceSet(obj, keyName, value) {
  propertyWillChange(obj, keyName);
  defineProperty(obj, keyName, value);
  propertyDidChange(obj, keyName);
  return value;
}

export default class extends BaseCell {
  didReceiveAttrs() {
    super.didReceiveAttrs(...arguments);

    const valuePath = get(this, 'column.valuePath');
    if (isArray(valuePath)) {
      forceSet(this, 'rawValue', collect(...valuePath));
    } else {
      forceSet(this, 'rawValue', reads(valuePath));
    }
  }
}

You can take a look at the twiddle demonstrating this. I am dead inside. 😂

But seriously, am I looking at this the wrong way? Why is such a basic thing so hard to do?

@buschtoens
Copy link
Collaborator Author

Work has begun here: ember-light-table-cell-type-multi-value

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