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

wrapComputed and computedDecoratorWithParams do not work with a getter/setter computed #404

Open
lolmaus opened this issue Feb 19, 2019 · 4 comments

Comments

@lolmaus
Copy link

lolmaus commented Feb 19, 2019

Hi!

This works:

  @computed('startDate')
  get startDateLuxon(): DateTime {
    return DateTime.fromJSDate(this.startDate, { zone: 'UTC' });
  }
  set startDateLuxon(luxonDate: DateTime) {
    this.set('startDate', luxonDate.toJSDate());
  }

But I would like to make this code reusable like this:

@dateToLuxon('startDate')
startDateLuxon!: DateTime;

I'm trying to achieve it with this:

import computed from 'ember-macro-helpers/computed';

// @ts-ignore
import { computedDecoratorWithParams } from '@ember-decorators/utils/computed';

export function dateToLuxonCP(key: any): ComputedProperty<DateTime> {
  return computed(key, {
    get(date: Date) {
      return Date2Luxon(date);
    },
    set(luxonDate: DateTime) {
      this.set(key, luxonDate.toJSDate());
    },
  });
}

// @ts-ignore
export const dateToLuxon = computedDecoratorWithParams((_desc: any, params: any[]) => dateToLuxonCP.apply(this, params));

It doesn't work! this.startDateLuxon returns undefined. I've tried putting a debugger into the getter, and it is never called.

I also tried this and it doesn't work either:

@wrapComputed(dateToLuxonCP('startDate'))
startDateLuxon!: DateTime;

CC @simonihmig.

@rwjblue
Copy link
Contributor

rwjblue commented Feb 19, 2019

Hmm, this seems more likely to be an issue with ember-macro-helpers doesn't it?

@pzuraq
Copy link
Contributor

pzuraq commented Feb 19, 2019

@lolmaus first off, @ember-decorators/utils is undocumented private/intimate API. Especially with the computedDecorator helpers, we've made breaking changes in the past without major version bumps, so definitely be aware and use them with caution.

As for the issue, if you wanted to drop ember-macro-helpers, this should work:

import computed from '@ember-decorators/object';

export function dateToLuxon(key: any) {
  return computed(key, {
    get() {
      return Date2Luxon(this[key]);
    },
    set(luxonDate: DateTime) {
      this.set(key, luxonDate.toJSDate());
    },
  });
}

If you want to continue using macro-helpers, then wrapComputed is the public API way to do this, like in your second example. In theory, as long as wrapComputed is passed a valid CP instance it should always work, so something strange is definitely going on if it doesn't.

@lolmaus
Copy link
Author

lolmaus commented Mar 1, 2019

@rwjblue: Hmm, this seems more likely to be an issue with ember-macro-helpers doesn't it?

@pzuraq: In theory, as long as wrapComputed is passed a valid CP instance it should always work

But what is wrong with ember-macro-helpers/computed?

I would like to note the following:

  1. ember-macro-helpers/computed has always served me well, I've never had any issues with it.
  2. ember-macro-helpers exists to fix issues with Ember API such as syntax inefficiencies and lack of CP composition.
  3. Together with ember-awesome-macros, it's my all-time favorite addon. I was expecting ember-decorators to replace it, but since ember-decorators is not gonna support CP composition by design, ember-macro-helpers is there to stay, and it seems to be the way of CP composition in Ember.
  4. ember-macro-helpers is quite an infrastructural addon. Numerous apps and several addons depend on it. Even ember-decorators itself used to depend on ember-macro-helpers.

I believe CP composition to be an important feature and humbly ask you to work together with @kellyselden to look into this. 🙇 🙏

PS I'm eager to help too but need some guidance.

@pzuraq
Copy link
Contributor

pzuraq commented Mar 1, 2019

@lolmaus totally understand the value behind ember-macro-helpers, not trying to dispute that. However, it's a complicated project that uses private Ember APIs to achieve what it's trying to do, and I just haven't had the time to dig in 😩

I can try to give you guidance where possible, but currently my focus is on landing decorators upstream in Ember, and making sure that doesn't completely break ember-macro-helpers (or figuring out the transition path forward)

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

No branches or pull requests

3 participants