-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Pre-work] Prepare @tracked decorator #16903
Conversation
Tracked tests
.vscode/settings.json
Outdated
@@ -0,0 +1,3 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't be commited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you for doing this. Just a few nitpicks, and I'd like to understand why in the tests we're initializing properties and then re-initializing them in the constructor to the same value. Unless I'm missing something, they're semantically identical.
packages/ember-metal/lib/tracked.ts
Outdated
): PropertyDescriptor { | ||
if ('value' in descriptor) { | ||
export function tracked(...dependencies: string[]): MethodDecorator; | ||
export function tracked(target: any, key: any): any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these can be typed as unknown
and PropertyKey
, respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
class Key { | ||
@tracked value = 'value'; | ||
constructor() { | ||
this.value = 'value'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to set this in the constructor if we're already initializing the property above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
'@tracked set', | ||
class extends AbstractTestCase { | ||
teardown() { | ||
setHasViews(() => false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently it's not. Deleted.
} | ||
} | ||
|
||
defineProperty( | ||
EmberObject.prototype, | ||
'full', | ||
computed('name', function() { | ||
let name = get(this, 'name'); | ||
let name = get(self, 'name'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're having to introduce the self
local variable because TypeScript can't infer the type of this
inside this computed property. If that's the case, a better way to do this would be to annotate the this
type:
computed('name', function(this: EmberObject) { /*...*/ })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Done.
Clean up tests, better types
Remove unneeded import
This is pre-work to expose
tracked
as a decorator. In this PR @smfoote moved the tests over from being a helper function that augmented the descriptors to an actual function that can be used as a decorator. This also updates all the tests to test@tracked
in static form instead of property descriptor helpers./cc @wycats @tomdale