Prevent element property set from throwing errors for readonly properties. #3687
Conversation
src/runtime/internal/dom.ts
Outdated
| } else if (key in node) { | ||
| } else if ( | ||
| //@ts-ignore | ||
| (a = Object.getOwnPropertyDescriptor(node.__proto__, key)) && a.set |
There was a problem hiding this comment.
It feels like it would be faster to just do const descriptors = Object.getOwnPropertyDescriptors(node.__proto__) once and then do descriptors[key] && descriptors[key].set here, but I don't actually have any data to back this up.
There was a problem hiding this comment.
Maybe you can't trust benchmarks but at least this supports your hunch, which makes sense to me. I certainly can't imagine it being any slower.
pngwn
force-pushed
the
3681-readonly-element-props
branch
2 times, most recently
from
86f2f57 to
c65e9fd
Compare
|
Can you add an entry under 'unreleased' in the changelog for this? I'm trying to start a thing where PRs from maintainers include that - and where maintainers will update the changelog right after merging something from a non-maintainer - so that cutting the release later is less of a hassle. |
src/runtime/internal/dom.ts
Outdated
| @@ -90,10 +90,12 @@ export function attr(node: Element, attribute: string, value?: string) { | |||
| } | |||
|
|
|||
| export function set_attributes(node: Element & ElementCSSInlineStyle, attributes: { [x: string]: string }) { | |||
| //@ts-ignore | |||
There was a problem hiding this comment.
Extreme nit zone: Every other place where we have @ts-ignore, there's a space after the //.
|
That's no problem, I'll sort these issues out shortly and squish them down. |
pngwn
force-pushed
the
3681-readonly-element-props
branch
from
c65e9fd to
712f85a
Compare
|
If you're happy with the changelog entry, this should be ready now. |
|
What typically has been happening was linking to the original issues in the changelog when possible rather than to the pull requests. |
pngwn
force-pushed
the
3681-readonly-element-props
branch
from
712f85a to
72d2c01
Compare
|
Ah right, okay. They're 50 / 50, so I wasn't sure. Fixed now. |
Fixes #3681.
When spreading props we were previously doing a
key in objcheck when deciding to assign to the property or set the attribute. This fails when the property exists but the setter isundefinedwhich is the case with some (all?) read-only properties.This PR changes that behaviour and instead uses
Object.getOwnPropertyDescriptor(el.__proto__, key)and, if it exists, checks that thesetproperty exists as well. It is important to provide two checks, one for the descriptors (or the key itself) and one for the actualsetproperty of the descriptor, to prevent throwing errors when trying to accesssetof undefined.I also needed to add
//@ts-ignorewhen accessingnode.__proto__since the TS people don't think valid properties should be on the node interface if they are deprecated.