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

Autotrack, take 2 #115

Merged
merged 2 commits into from
Feb 22, 2018
Merged

Autotrack, take 2 #115

merged 2 commits into from
Feb 22, 2018

Conversation

wycats
Copy link
Contributor

@wycats wycats commented Feb 17, 2018

This PR adds "auto-tracking" to tracked getters.

Previously, when defining a tracked computed property, you would write:

class {
  @tracked first;
  @tracked last;

  @tracked('first', 'last') get name() {
    return `${this.first} ${this.last}`;
  }
}

After this change, you can leave off the "dependent keys" and write:

class {
  @tracked first;
  @tracked last;

  @tracked get name() {
    return `${this.first} ${this.last}`;
  }
}

Rather than rely on knowing the full list of dependent properties up front, after this change Glimmer tracks the consumption of all @tracked properties inside the getter, and automatically creates a validator based on the consumed keys.

@mmun
Copy link
Member

mmun commented Feb 17, 2018

The code looks good to me. I've also written a benchmark here:

https://github.com/glimmerjs/glimmer.js/compare/autotrack...autotrack-benchmark?expand=1#diff-504d4e1545bcaa7fc620aec828f900b4

The results are great! There's basically no difference between @tracked with explicit dependencies and with auto-tracking.

screen shot 2018-02-17 at 2 57 07 am

screen shot 2018-02-17 at 2 56 41 am

@locks
Copy link
Contributor

locks commented Feb 17, 2018

Yay 😍

Copy link
Contributor

@tomdale tomdale left a comment

Choose a reason for hiding this comment

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

Implementation looks 👌 to me. @wycats, would you mind updating the comments and inline documentation to reflect the code changes? Happy to merge after that's done.

@tomdale
Copy link
Contributor

tomdale commented Feb 20, 2018

Also, ideally, would you mind updating the body of the PR to explain to viewers at home what's going on here?

@wycats
Copy link
Contributor Author

wycats commented Feb 21, 2018

@tomdale done!

@rwjblue rwjblue merged commit b10a276 into master Feb 22, 2018
@rwjblue rwjblue deleted the autotrack branch February 22, 2018 13:40
@sandstrom
Copy link

Out of curiosity, would this work too? 😄

class {
  first: '',
  last: '',

  @tracked get name() {
    return `${this.first} ${this.last}`;
  }
}

In other words, is it enough to tell the computed property that it's tracked, or must all dependent keys be marked as tracked too?

@rwjblue
Copy link
Member

rwjblue commented Mar 8, 2018

Anything not tracked is considered "constant", so in the above if you expect to update on changes from first or last you would need to track those also...

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

Successfully merging this pull request may close these issues.

None yet

6 participants