This repository has been archived by the owner on Jan 20, 2019. It is now read-only.
RFC for caching results of treeFor hook
#90
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ember CLI: Production code stripping
11 tasks
stefanpenner
approved these changes
Dec 15, 2016
| ``` | ||
| The default implementation for `Addon.prototype.cacheKeyForTree` will: | ||
|
|
||
| - Utilize a shared NPM package (e.g. `calculate-cache-key-for-tree`) that will generate a cache key that incorporates at least the following pieces of information: |
There was a problem hiding this comment.
An additional or complementary heuristic may be that they are at the exact same place on disk?
There was a problem hiding this comment.
@stefanpenner - Ya, I'm happy to add that also.
There was a problem hiding this comment.
@rwjblue only if you think it adds value. Just wanted to share, in-case it was overlooked.
trentmwillis
changed the title
treeFor hook results.treeFor hook
homu
added a commit
to ember-cli/ember-cli
that referenced
this pull request
Dec 16, 2016
Implement addon treeFor caching. In Ember CLI today, all addons at each level are built through the standard `treeFor` / `treeFor*` hooks. These hooks are responsible for preprocessing the JavaScript included by the tree returned from that specific hook (e.g., `treeForAddon` preprocesses the JS for the addon tree). [This RFC](ember-cli/rfcs#90) proposes a mechanism that would allow these returned trees to be cached by default (when no build time customization is done) and expose proper hooks for addon authors to control the degree to which we dedupe these trees. See [ember-cli/rfcs#90](ember-cli/rfcs#90) for more context. *Note:* This effort was derived from @stefanpenner's work in #6429. ### TODO: - [x] Unit test `cacheKeyForTree` implementation - [x] Confirm that `treeFor*` method overrides opt-out of caching - [x] Confirm that modifying `this.treeForMethods` opts out of caching. - [x] Confirm that repeated calls to `cacheKeyForTree` return the same value. - [x] Unit test `treeFor` caching mechanism - [x] Extract basic `cacheKeyForTree` implementation to stand alone utility function - [x] test ### Future TODO: - [ ] Ensure `mergeTrees` macro utility function is updated to properly dedupe when called like `mergeTrees([treeA, treeA])`. (#6574) - [ ] Updates to Broccoli to ensure that "reused / previously built trees" are represented properly in heimdall tree. This will included confirming that `buildSteps` properly handles the deduplicated results and shows an improvement. - [ ] Extract the default `cacheKeyForTree` implementation into a separate package. - [ ] Add API docs for `Addon.prototype.cacheKeyForTree` (in a separate PR after the corresponding RFC lands, so that ember-cli.com/api does not show the experimental API's until the RFC lands).
trentmwillis
approved these changes
Jan 10, 2017
There was a problem hiding this comment.
Looks good to me. Only potential suggestion would be mentioning the cache API and maybe link to cache-key-for-tree.
This was referenced Jan 24, 2017
|
This was promoted to Final Comment Period in our team meeting. |
trentmwillis
pushed a commit
to trentmwillis/ember-cli
that referenced
this pull request
Feb 1, 2017
As discussed in ember-cli/rfcs#90
|
Discussed at ember-cli core team meeting today. No new information was brought up during the final comment period, and all of the ember-cli core team is in favor. Merging... |
homu
added a commit
to ember-cli/ember-cli
that referenced
this pull request
Feb 2, 2017
Promote cacheKeyForTree to public API As discussed in ember-cli/rfcs#90. Should be good to go once the RFC is merged. Pro tip: use `?w=1` when viewing the changes to tidy up the large amount of whitespace changes in the tests.
rwjblue
added a commit
to rwjblue/data
that referenced
this pull request
Aug 1, 2017
This hook was added in [ember-cli/rfcs#90](https://github.com/ember-cli/rfcs/blob/master/complete/0090-addon-tree-caching.md), and essentially guarantees that a given tree returned by an addon is considered stable (or not). In the case of Ember Data, we do not have different tree output based on our parent (project or addon). Specifically, this implementation allows both an app _and_ a lazy engine to depend on `ember-data` without duplicating the `ember-data` assets in both the `assets/vendor.js` and `engine-dist/<engine-name>/assets/engine-vendor.js`.
rwjblue
added a commit
to rwjblue/data
that referenced
this pull request
Aug 10, 2017
This hook was added in [ember-cli/rfcs#90](https://github.com/ember-cli/rfcs/blob/master/complete/0090-addon-tree-caching.md), and essentially guarantees that a given tree returned by an addon is considered stable (or not). In the case of Ember Data, we do not have different tree output based on our parent (project or addon). Specifically, this implementation allows both an app _and_ a lazy engine to depend on `ember-data` without duplicating the `ember-data` assets in both the `assets/vendor.js` and `engine-dist/<engine-name>/assets/engine-vendor.js`.
kategengler
pushed a commit
to kategengler/rfcs
that referenced
this pull request
Jan 19, 2019
RFC for caching results of `treeFor` hook
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rendered
Implementation PR here.