-
Notifications
You must be signed in to change notification settings - Fork 319
Removes wrapping of properties in favor of always respecting noAccessor
#454
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
Conversation
…sor` Fixes #450. This removes all wrapping of existing accessors. This changes makes property generation simpler and more straightforward. If an accessor should not be generated, for example when there is a user defined accessor, users should set the `noAccessor` flag to true.
…rototype This means `noAccessor` is only required when extending a user defined accessor with new metadata.
kevinpschaaf
left a comment
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.
Approved, with some code comment suggestions.
| clazz.createProperty(element.key, options); | ||
| } | ||
| }; | ||
| // createProperty() takes care of defining the property, but we still |
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.
move comment to below the else
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.
Done.
src/lib/updating-element.ts
Outdated
| enumerable : true | ||
| }; | ||
| } else { | ||
| if (!options.noAccessor && !this.prototype.hasOwnProperty(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 an early return would read nicer here
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.
Done.
src/lib/updating-element.ts
Outdated
| set(value: any) { | ||
| const oldValue = this[name]; | ||
| this[key] = value; | ||
| const desc = { |
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.
you can inline this into the defineProperty call now
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.
Done.
Co-Authored-By: sorvell <sorvell@google.com>
Co-Authored-By: sorvell <sorvell@google.com>
Since we no longer wrap existing accessors, the language and example in #accessors-custom is updated to indicate the user must manually call `requestUpdate`. In addition, since `noAccessor` is _only_ needed now in a pretty esoteric case (extending a superclass and changing metadata for a declared property _that also has a custom accessor_), the code example in that section is simplified.
…#460) * Update docs to reflect #454 change to remove accessor wrapping Since we no longer wrap existing accessors, the language and example in #accessors-custom is updated to indicate the user must manually call `requestUpdate`. In addition, since `noAccessor` is _only_ needed now in a pretty esoteric case (extending a superclass and changing metadata for a declared property _that also has a custom accessor_), the code example in that section is simplified. * Code samples for accessors with subclassing * Update per feedback
Fixes #450. This removes all wrapping of existing accessors. This changes makes property generation simpler and more straightforward. If an accessor should not be generated, for example when there is a user defined accessor, users should set the
noAccessorflag to true.