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

Implement @glimmer/tracking/primatives/cache (see [emberjs/rfcs#615](https://github.com/emberjs/rfcs/blob/master/text/0615-autotracking-memoization.md) for details) #1090

Merged
merged 1 commit into from
May 13, 2020

Conversation

pzuraq
Copy link
Member

@pzuraq pzuraq commented May 10, 2020

Adds the cache API specified in RFC 615.

There were a few minor refactors in addition to this:

  • Moved trackedData into a separate module, since the tracking
    module was getting pretty big.

  • Renamed isConst to isConstTagged, since there is a naming conflict
    with the new isConst function. Given isConstTagged will be removed
    soon and is not generally much used outside of the VM, I thought it
    was better to prioritize the new API getting this name.

  • Rewrote the memoizeTracked implementation on top of the cache APIs.
    One thing worth noting is that in order to support the API
    memoizeTracked had before, getValue also needed to be able to
    accept and pass on args. This is something we'll wrap to hide when we
    expose this publicly in Ember/Glimmer for the time being, since that
    was a capability that was not added in the RFC.

@pzuraq pzuraq force-pushed the feat/adds-cache-api branch from 991e18b to cfccc14 Compare May 10, 2020 17:17
@rwjblue
Copy link
Member

rwjblue commented May 11, 2020

@pzuraq - Looks like this needs a rebase

packages/@glimmer/validator/lib/tracking.ts Show resolved Hide resolved
packages/@glimmer/validator/lib/tracking.ts Outdated Show resolved Hide resolved
packages/@glimmer/validator/lib/tracking.ts Outdated Show resolved Hide resolved
packages/@glimmer/validator/lib/tracking.ts Outdated Show resolved Hide resolved
@pzuraq pzuraq force-pushed the feat/adds-cache-api branch from cfccc14 to d1064b3 Compare May 11, 2020 23:46
Copy link
Contributor

@krisselden krisselden left a comment

Choose a reason for hiding this comment

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

Make an exported interface CacheHandle<TResult = unknown> { __isCacheBrand: TResult } that createCache casts Cache to and getValue and isConst take.

The Cache doesn't implement this, you just cache at boundaries (you cast Cache as unknown then the CacheHandle in createCache and the CacheHandle to unknown to Cache in getValue and isConst).

This lets the implementation change independent of type consumers.

packages/@glimmer/validator/lib/tracking.ts Outdated Show resolved Hide resolved
packages/@glimmer/validator/lib/tracking.ts Outdated Show resolved Hide resolved
@pzuraq pzuraq force-pushed the feat/adds-cache-api branch 2 times, most recently from df5733d to d944a43 Compare May 12, 2020 20:58
@rwjblue rwjblue requested review from rwjblue and krisselden May 12, 2020 21:11
packages/@glimmer/validator/lib/tracking.ts Outdated Show resolved Hide resolved
packages/@glimmer/validator/lib/tracking.ts Show resolved Hide resolved
Adds the cache API specified in [RFC 615](https://github.com/emberjs/rfcs/blob/master/text/0615-autotracking-memoization.md).

There were a few minor refactors in addition to this:

- Moved `trackedData` into a separate module, since the `tracking`
  module was getting pretty big.

- Renamed `isConst` to `isConstTagged`, since there is a naming conflict
  with the new `isConst` function. Given `isConstTagged` will be removed
  soon and is not generally much used outside of the VM, I thought it
  was better to prioritize the new API getting this name.

- Rewrote the `memoizeTracked` implementation on top of the cache APIs.
  One thing worth noting is that in order to support the API
  `memoizeTracked` had before, `getValue` also needed to be able to
  accept and pass on args. This is something we'll wrap to hide when we
  expose this publicly in Ember/Glimmer for the time being, since that
  was a capability that was not added in the RFC.
@pzuraq pzuraq force-pushed the feat/adds-cache-api branch from 01acfbe to 790508c Compare May 13, 2020 23:28
@rwjblue rwjblue merged commit e8e2fc6 into master May 13, 2020
@pzuraq pzuraq deleted the feat/adds-cache-api branch May 14, 2020 02:11
@rwjblue rwjblue changed the title [FEATURE] Adds cache api Implement @glimmer/tracking/primatives/cache (see [emberjs/rfcs#615](https://github.com/emberjs/rfcs/blob/master/text/0615-autotracking-memoization.md) for details) May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants