-
Notifications
You must be signed in to change notification settings - Fork 48.8k
[idea] Automatically detect property/attribute for HTMLDOMPropertyConfig #2141
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
cc @zpao |
hasPropertyAccessorCache[nodeName] = nodeHasPropertyAccessors = {}; | ||
} | ||
if (!(prop in nodeHasPropertyAccessors)) { | ||
testElement = document.createElement(nodeName); |
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.
Updates are going to get slow until this cache is populated, which I think might be a showstopper. One thing we can do is hold onto our testElement
for each node type so we aren't recreating each time we call the function. That's still going to be slower than what we have now though. The tradeoff is a bit more correctness and not having to special case things (as much).
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.
@zpao Last I checked, document.createElement
is crazy fast, even on IE8, we're talking 100K/s+ unless my memory fails me. We talked about caching nodes for getDefaultForProperty
but cancelled that because it seemed pointless.
I don't mind caching, but I really really doubt the cost of this can be measured at all unless there is thrashing involved. If that is, then it could probably be avoided by simply keeping a reference to the last element which should provide sufficient caching. Your call :)
Additionally, the cost of this should be immeasurable when compared to the cost of actually figuring out what to update.
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'm suspicious of any call to document.*, especially in a tight loop. I'd like to see perf in a real-life pathological case with lots of different types of elements and properties that all need to be looked up.
also, cc @yungsters
Since properties can have all kind of weird behaviors and formats that attributes usually doesn't, I think that properties should probably be opt-in on a whitelist. Browsers add crazy new properties all the time. It seems fragile to use those. Instead, we could, by default, opt to always use attributes for unknown props. That way we're compatible with most of the arbitrary attributes without a whitelist of special cases. |
@sebmarkbage I expanded on this in a related issue a short while back: #1449 (comment), which I think is a preferable direction. Full auto detection is, as you say, probably rather fragile. |
I’m closing this as it doesn’t appear that we want to go through with it, or at least not at the moment. Thank you for your work on this PR! |
Proper implementation of #1512
It does not get rid of
MUST_USE_PROPERTY
(see #2140, #2202) andMUST_USE_ATTRIBUTE
because I'm not 100% sure if we might want to use it or not, but I assume there's no need for it.I also removed a test that I'm not sure what to do about. It relied on really hacky behavior and I don't really see a simple way of keeping it alive (not really familiar with testing), but please shout if you think otherwise.
This is beneficial for a lot of reasons, one being that we ensure DOM attributes always reflect the intended value.
MUST_USE_PROPERTY
was previously used for some attributes not supported by all browsers/elements, which when updated would just add a custom property to the node object rather than the intended attribute.Half-related: #1848 and #1657