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

Deprecate recompute on Helper? #633

Open
chancancode opened this issue May 27, 2020 · 5 comments
Open

Deprecate recompute on Helper? #633

chancancode opened this issue May 27, 2020 · 5 comments

Comments

@chancancode
Copy link
Member

chancancode commented May 27, 2020

The point of the recompute method is that you call it to trigger Ember to re-render the helper in some kind of callback, such as observers, promise callbacks, etc.

Since helpers are now auto-tracked, it should be possible to refactor any usage of recompute into setting tracked state and accomplish the same thing. In most cases, the transformation is pretty straightforward.

Before:

class MyHelper extends Helper {
  @service foo;

  @observes('foo.bar.[]') onBarChanged() {
    this.recompute();
  }

  compute() {
    return this.foo.bar.map(...);
  }
}

After:

class MyHelper extends Helper {
  @service foo;

  @tracked bar = this.foo.bar;

  @observes('foo.bar.[]') onBarChanged() {
    this.bar = this.foo.bar;
  }

  compute() {
    return this.bar.map(...);
  }
}

The example uses an observer, because that is a common kind of callback used in this position, but it's ultimately unrelated to my point.I also think there are better ways to accomplish the same thing without using observers, in general, but that is also besides the point here.

For prosperity, here is a second example that doesn't use observers:

Before:

const UNINITIALIZED = Object.freeze({});

class AwaitHelper extends Helper {
  resolved = undefined;
  promise = UNINITIALIZED;

  compute([promise]) {
    if (promise !== this.promise) {
      this.resolved = undefined;
      this.promise = promise;

      Promise.resolve(promise).then(resolved => {
        if (promise === this.promise) {
          this.resolved = resolved;
          this.recompute();
        }
      });
    }

    return this.resolved;
  }

  willDestroy() {
    this.promise = UNINITIALIZED;
  }
}

After:

const UNINITIALIZED = Object.freeze({});

class AwaitHelper extends Helper {
  @tracked resolved = undefined;
  promise = UNINITIALIZED;

  compute([promise]) {
    if (promise !== this.promise) {
      this.resolved = undefined;
      this.promise = promise;

      Promise.resolve(promise).then(resolved => {
        if (promise === this.promise) {
          this.resolved = resolved;
        }
      });
    }

    return this.resolved;
  }

  willDestroy() {
    this.promise = UNINITIALIZED;
  }
}

The general pattern is: instead of calling recompute(), just refactor into setting a piece of tracked state and use that tracked state in the compute() method.

Would be interested to see if there are any valid use cases this doesn't cover.

Copy link
Member

rwjblue commented May 27, 2020

I think it is a tad difficult to suggest "just use @tracked" at the moment, since we also expect addons to continue supporting ember-source@3.12 which doesn't support tracking concepts. It is also non-trivial to write code that works for both styles (without completely duplicating the helper).

Either way, I agree with this direction. My point above is that we should probably tie this deprecation to land no earlier than 3.21, at which point 3.12 will no longer be an active concern (since 3.16 and 3.20 would be the active LTS versions). Note: 3.21 is current ember-source master, so this is still a great time to push this forward. 😄

@chancancode
Copy link
Member Author

3.21 is current ember-source master

What a time to be alive 😄

@mixonic
Copy link
Sponsor Member

mixonic commented Jul 8, 2022

We discussed this again today at the framework meeting. As the tracked system is now very, very common, this proposal is definitely ready for RFC and advancement.

@locks locks added the S-Exploring In the Exploring RFC Stage label Jul 15, 2022
@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

@mixonic who will write the RFC?

@wagenet
Copy link
Member

wagenet commented Jul 29, 2022

Should this be considered with #832 or separately?

@kategengler kategengler removed the S-Exploring In the Exploring RFC Stage label Aug 4, 2023
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

6 participants