[reactive-element] Always wrap user accessors#4146
Conversation
🦋 Changeset detectedLatest commit: 3e85612 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultsthis-change
render
update
update-reflect
this-change, tip-of-tree, previous-release
render
update
nop-update
this-change, tip-of-tree, previous-release
render
update
this-change, tip-of-tree, previous-release
render
update
update-reflect
|
|
The size of lit-html.js and lit-core.min.js are as expected. |
69e2ac1 to
89e513e
Compare
| _foo?: number; | ||
| updatedContent?: number; | ||
|
|
||
| @property({reflect: true, type: Number}) |
There was a problem hiding this comment.
How does this code behave with this change? Is it a breaking change?
There was a problem hiding this comment.
Er, I should say before this change
There was a problem hiding this comment.
It's not a breaking change with experimentalDecorators: true because those read the entire property descriptor off the class and it doesn't matter which is decorated.
Standard decorator usage requires that we decorate the setter though, because we're only passed the setter method.
I can test both in legacy tests.
There was a problem hiding this comment.
Added a test for decorating a getter in experimental decorators. I don't think we can detect which accessor was decorated, so I couldn't add a deprecation warning.
scripts/check-size.js
Outdated
| // | ||
| // In either case, update the size here and push a new commit to your PR. | ||
| const expectedLitCoreSize = 15023; | ||
| const expectedLitCoreSize = 15135; |
There was a problem hiding this comment.
Huh, bigger change than I'd have expected from this PR
There was a problem hiding this comment.
Yeah... there's a bit of change here. I'm going to change the approach slightly, though I don't think it'll be fewer bytes
There was a problem hiding this comment.
my new change made it worse: 15214 bytes, but I think aside from that it's a better implementation.
There was a problem hiding this comment.
Ok, kept it more static and got the size down to 15115
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| get(): any { | ||
| return (this as {[key: string]: unknown})[key as string]; | ||
| return get === undefined |
There was a problem hiding this comment.
actually... I want to change this so that the conditional is outside of the accessors and run only once
Co-authored-by: Benny Powers <bennypowers@users.noreply.github.com>
| // user-defined accessors. Note that if the super has an accessor we will | ||
| // still overwrite it | ||
| if (!options.noAccessor && !this.prototype.hasOwnProperty(name)) { | ||
| if (!options.noAccessor) { |
There was a problem hiding this comment.
For my own understanding, is this where the wrapping is happening? !this.prototype.hasOwnProperty(name) would previously only wrap class fields and skip accessors or user defined get/set.
Now explicit getters/setters and accessor keyword cases pass through this code path (in addition to the class fields).
| @@ -728,21 +729,22 @@ export abstract class ReactiveElement | |||
| key: string | symbol, | |||
There was a problem hiding this comment.
Does the comment on this method need to be updated? Specifically the part: If no descriptor is returned, the property will not become an accessor.
I think it needs to be updated to mention that it now wraps accessors.
There was a problem hiding this comment.
That comment is still true because of the if (descriptor !== undefined) check in createProperty
AndrewJakubowicz
left a comment
There was a problem hiding this comment.
Nice! The tests are really great as well!
| // Check initial values | ||
| assert.equal(el._foo, undefined); | ||
| assert.equal(el.updatedContent, undefined); | ||
| assert.equal(el.updatedProperties!.get('foo'), undefined); |
There was a problem hiding this comment.
Here and in the main decorators tests: should we assert on el.updatedProperties!.has('foo') here?
There was a problem hiding this comment.
Since there's no initializer it'll be false, but I can assert on that instead
Part of #4144
This makes all reactive properties - static properties block, experimental decorators, and standard decorators - wrap user written accessors and call
this.requestUpdate()from the generated setter.