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

Adds @cached decorator #358

Merged
merged 4 commits into from
Aug 9, 2022
Merged

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Aug 24, 2021

This is the same implementation as ember and the polyfill

Copy link

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

emberjs/rfcs#566

Beautiful!

@pzuraq
Copy link
Member

pzuraq commented Aug 25, 2021

I think this needs to be implemented in Ember as well, since Ember has its own implementation of @glimmer/tracking for the moment still

@NullVoxPopuli
Copy link
Contributor Author

@pzuraq why is that? is it worth using this effort to make ember use this @glimmer/tracking?

@NullVoxPopuli
Copy link
Contributor Author

ah looks like there is computed interop in there 🤔
alright, I'll do a separate PR to emberjs for @cached Though, I wonder if it can re-export this cached? 🤔

Copy link

@tehhowch tehhowch left a comment

Choose a reason for hiding this comment

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

Would you mind adding tests for using the decorator with private getters? Or is caching a private getter expected to never be supported?

Ref: ember-polyfills/ember-cached-decorator-polyfill#93

@NullVoxPopuli
Copy link
Contributor Author

Afaik, It's not possible with today's decorators because of how the transpiling happens.

But it should work with the new decorators proposal, afaik. @pzuraq would have details. ;)

@NullVoxPopuli
Copy link
Contributor Author

looks like the errors here in CI are present on every other PR 🎉

@NullVoxPopuli
Copy link
Contributor Author

Ember version: emberjs/ember.js#19772

@NullVoxPopuli NullVoxPopuli force-pushed the cached-decorator branch 2 times, most recently from 427fe2a to 62f6dc2 Compare October 5, 2021 15:34
@mydea
Copy link

mydea commented Jan 3, 2022

Since this has landed in ember, would it be possible to merge this as well? As of now, e.g. when using Typescript it is a bit akward, since cached types do not exist in @glimmer/tracking but can actually be imported from that path 😅

@rwjblue
Copy link
Member

rwjblue commented Jan 3, 2022

Ya, we should definitely get this landed.

@NullVoxPopuli - Can you rebase to kick off a new set of tests?

@NullVoxPopuli
Copy link
Contributor Author

it's already up to date / fully rebased

@NullVoxPopuli
Copy link
Contributor Author

@rwjblue any way you/we/i can click the "re-run all jobs button"?

@rwjblue
Copy link
Member

rwjblue commented Feb 1, 2022

Hmm, no? There is no "re-run all" button on the checks tab.

@NullVoxPopuli NullVoxPopuli mentioned this pull request Feb 24, 2022
@ef4 ef4 merged commit 4707c1c into glimmerjs:master Aug 9, 2022
@knownasilya
Copy link

🎉

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.

None yet

9 participants