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

Document the caching of autotracked properties #1474

Merged
merged 1 commit into from
Aug 8, 2020

Conversation

YoranBrondsema
Copy link
Contributor

@ijlee2 ijlee2 self-requested a review July 7, 2020 20:36
@ijlee2
Copy link
Member

ijlee2 commented Jul 7, 2020

Hi, @YoranBrondsema. I appreciate your looking into the current state for caching getters. I didn't know about ember-cache-primitive-polyfill before, so I learned something new from your PR today.

I thought that the code examples were clear and illustrated the effect of caching well. I think we can shorten the surrounding texts a little (we can come back to this suggestion later).

Potential problem

Right now, I'm unable to implement your example as a component in a new Ember 3.19 app. When I introduced the import statement for the polyfill, I got the error,

Can't resolve '@glimmer/tracking/primitives/cache' in '/Users/isaac/Desktop/hello-world'

I believe it's the problem reported in ember-polyfills/ember-cache-primitive-polyfill#3.

I reached out to @pzuraq on Discord to see if the fix is something that we can anticipate soon. If not, I wonder if the Guides today should show the polyfill solution, or if we can present a native JS solution instead (if this is simple to do).

Please let us know what you think. Thank you! 🙂

@YoranBrondsema
Copy link
Contributor Author

@ijlee2 Thanks for your prompt reply.

I have to be honest with you, I didn't even try ember-cache-primitive-polyfill but went straight to using @cached from tracked-toolbox for my own app (which runs in production). I 100% agree that we can't publish a guide for something that doesn't work.

I suggest we await @pzuraq's input and I imagine it'll work when ember-polyfills/ember-cache-primitive-polyfill#3 is resolved.

@ijlee2
Copy link
Member

ijlee2 commented Jul 8, 2020

Thanks for understanding. I also think it's a good idea to wait for the issue to be resolved. I think Chris and others are more aware of the issue since yesterday so I'm hopeful that it will be resolved soon.

@pzuraq
Copy link
Contributor

pzuraq commented Jul 13, 2020

I believe this API should be released in 3.20, so we can probably remove the text about using the polyfill and land this guide once it is available. We should be able to confirm with the current beta.

@YoranBrondsema
Copy link
Contributor Author

Cool, I removed the mention of the polyfill. Right now, it refers to the RFC. Once it lands, I imagine it should refer to the doc in the API docs (https://api.emberjs.com/) instead?

@ijlee2
Copy link
Member

ijlee2 commented Jul 19, 2020

@YoranBrondsema Apologies for a late reply.

For v3.15-3.19 Guides, I think I'd prefer that we present a working solution for caching getters, something that developers can use now. We would continue to use your full name example to demo the solution.

When the API is officially a part of Ember (say, it's v3.20 as an example), we can link to the API docs for more information, yep! (Or to the RFC if the API docs are yet to be updated.)

Discussion items:

What are your thoughts on my preference for presenting a working solution?

Besides using @computed, off the top of my head, I don't know a solution to caching getters with vanilla JavaScript. It should be doable, I believe, so I need to spend some time on research. (It's possible that @pzuraq already presented a solution in a blog post. ☺️)

If you know of a solution, can you update your full name example to show us how?

@rwjblue
Copy link
Contributor

rwjblue commented Jul 22, 2020

I believe this API should be released in 3.20, so we can probably remove the text about using the polyfill and land this guide once it is available. We should be able to confirm with the current beta.

Actually, the internals are available in 3.20 but the feature itself is not exposed there just yet. Needs feature flagging and whatnot (so likely 3.22 at the earliest for usage without the polyfill).

@YoranBrondsema
Copy link
Contributor Author

YoranBrondsema commented Aug 2, 2020

Thanks for all your feedback. A quick summary to make it easier:

3.20 to 3.22? --> mention polyfill

From 3.20 to whenever the feature is available without a feature flag (possibly 3.22), we should mention that the polyfill ember-cache-primitive-polyfill is required.

3.15 to 3.19 --> also mention polyfill?

The doc for ember-cache-primitive-polyfill mentions that it's compatible with Ember 3.13 and up. So I imagine we can use the same guide for 3.15 all the way to 3.22? Or is anything that opposes the use of the caching polyfill for 3.15 to 3.19?

Release of feature and beyond --> remove mention of polyfill

We remove the mention of the polyfill when the caching feature is available in Ember.js without a feature flag.

Release of @cached RFC --> replace primitive caching API with @cached decorator

The @cached decorator is a lot simpler to use for developers than the primitive caching API.

What are you thoughts @ijlee2 @pzuraq @rwjblue ?

@rwjblue
Copy link
Contributor

rwjblue commented Aug 2, 2020

FYI - The @glimmer/tracking/primitives/cache API has been enabled by default over in emberjs/ember.js#19067 and is currently slated for release as part of 3.22.0 (assuming everything goes well on canary + beta).

@ijlee2
Copy link
Member

ijlee2 commented Aug 3, 2020

@YoranBrondsema Thanks for creating a suggested path forward.

I think your plan is sound and suggesting the polyfill for 3.15-3.19 works. (The import issue doesn't seem to have been addressed yet. However, I don't want that to be a blocker for your PR.)

If I understood Robert's last comment correctly, it seems we can enable a feature flag starting 3.20? If so, I think we can document the use of the flag instead of the polyfill (from 3.20 until release of feature).

Let me try out the feature flag after work today.

@rwjblue
Copy link
Contributor

rwjblue commented Aug 3, 2020

If I understood Robert's last comment correctly, it seems we can enable a feature flag starting 3.20? If so, I think we can document the use of the flag instead of the polyfill (from 3.20 until release of feature).

Sorry I wasn't very clear, the feature flag is enabled by default and will be included in ember-source >= 3.22.0-beta.1 when that is released (~ 3 or 4 weeks from now). For Ember itself, you cannot enable feature flags on any release other than a canary build (this ensures that experimental / unstable features do not sneak into any tagged releases) which means you can't use the caching primitives in Ember until 3.22+.

So basically for 3.13 through 3.21, we should mention / use the polyfill; for 3.22+ we should remove mention of the polyfill and use the native API directly.

@ijlee2
Copy link
Member

ijlee2 commented Aug 3, 2020

@rwjblue Thanks for clarifying!

@YoranBrondsema May I review your current changes once more later today and make suggestions? Afterwards, you can apply the suggestions (feel free to suggest something else) and backport the text from release folder to v3.15.0-v3.19.0 folders. (I think you'll need to rebase your branch so that you see the guides/v3.19.0 folder.)

Comment on lines 359 to 363
This works fine in most cases. But sometimes you want a bit more control over
the recomputation, for instance if the computation that happens in the getter
is very expensive. In that case, you want to cache the value in between calls
when the dependencies haven't changed. You want to recompute only if one of the
dependencies has been modified.
Copy link
Member

@ijlee2 ijlee2 Aug 4, 2020

Choose a reason for hiding this comment

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

Suggested change
This works fine in most cases. But sometimes you want a bit more control over
the recomputation, for instance if the computation that happens in the getter
is very expensive. In that case, you want to cache the value in between calls
when the dependencies haven't changed. You want to recompute only if one of the
dependencies has been modified.
From the value of `count`, we see that `aspectRatio` was calculated 3 times.
Recomputing is fine in most cases. If the computation that happens in the getter
is very expensive, however, you will want to cache the value and retrieve it when
the dependencies haven't changed. You want to recompute only if a dependency
has been updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good.

Copy link
Member

@ijlee2 ijlee2 left a comment

Choose a reason for hiding this comment

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

Hi, @YoranBrondsema. I reread the paragraphs and code examples that you had written. They provided a great starting point so I appreciate your work and patience in getting this PR through. 🧡

For each paragraph, I made a suggestion that I think would be concise and easy to understand for someone who is new to caching getters or using Ember's cache API. You are welcome to apply the suggestions as they are, or leave a comment to ask questions or to suggest an alternative text.

For code, I do need us to show an example that doesn't involve first and last names, to help address an active issue (#1480). I think the Photo class and calculating the aspect ratio (some math to stand for an expensive computation) would make a good example. Will this be okay with you?

As a reminder, once you make changes to the autotracking-in-depth.md file in the release folder, please backport the changes to:

  • v3.15.0
  • v3.16.0
  • v3.17.0
  • v3.18.0
  • v3.19.0

Let us know if you have questions or need help with writing. ☺️

console.log(person.fullName); // Jennifer Weber
console.log(count); // 2;
```

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
From the value of `count`, we see that, this time, `aspectRatio` was calculated only twice.

@YoranBrondsema
Copy link
Contributor Author

For each paragraph, I made a suggestion that I think would be concise and easy to understand for someone who is new to caching getters or using Ember's cache API. You are welcome to apply the suggestions as they are, or leave a comment to ask questions or to suggest an alternative text.

I agree with your changes, nice.

For code, I do need us to show an example that doesn't involve first and last names, to help address an active issue (#1480). I think the Photo class and calculating the aspect ratio (some math to stand for an expensive computation) would make a good example. Will this be okay with you?

Yes!

I updated the text following your suggestions. Once I get your OK, I'll:

  • backport the file to 3.15 to 3.19
  • squash this PR into a single commit

I think we'll be good to go after these two steps.

@ijlee2
Copy link
Member

ijlee2 commented Aug 6, 2020

@YoranBrondsema Thanks for reviewing my suggestions and updating the texts. I think they read great and we're ready to backport them to 3.15-3.19!

@YoranBrondsema
Copy link
Contributor Author

OK should be good now @ijlee2.

Copy link
Member

@ijlee2 ijlee2 left a comment

Choose a reason for hiding this comment

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

Thanks! I think the PR can be merged now.

I may want to think about the subsection title more. (Is it better to say caching getters or caching tracked properties? Is there no difference?)

The Autotracking In-Depth section will need to be updated soon with more content (show how to update arrays and objects), so I think we can address this subsection's title at a later time.

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.

4 participants