perf: Use weakmap for dom data #6103
Conversation
|
Huh, apparently, WeakMap is supported in IE11. Though, it's only basic support, so, we'd want to double check that the operations we are using are supported in IE11. |
brandonocasey
force-pushed
the
perf/use-weakmap
branch
from
d6e89cb
to
50e8398
Compare
brandonocasey
force-pushed
the
perf/use-weakmap
branch
from
50e8398
to
32cd08f
Compare
| @@ -250,7 +253,11 @@ export function on(elem, type, fn) { | |||
| return _handleMultipleEvents(on, elem, type, fn); | |||
| } | |||
|
|
|||
| const data = DomData.getData(elem); | |||
| if (!DomData.has(elem)) { | |||
There was a problem hiding this comment.
I see a couple usages of if !has, set, then get. Do you think it's worth a helper?
There was a problem hiding this comment.
I think that it might not make sense in this case since it is only used in two places.
EDIT: And the helper name will be likely have to be longer than duplicating the code would be.
There was a problem hiding this comment.
Yeah, you're right. If we end up with this a few more times, then it'll make sense.
|
This is good to go but I think I'm going to hold off on this to be a minor change. |
gkatsev
added
minor
| @@ -250,7 +253,11 @@ export function on(elem, type, fn) { | |||
| return _handleMultipleEvents(on, elem, type, fn); | |||
| } | |||
|
|
|||
| const data = DomData.getData(elem); | |||
| if (!DomData.has(elem)) { | |||
There was a problem hiding this comment.
Yeah, you're right. If we end up with this a few more times, then it'll make sense.
…me people have relied on this (#6216) When a player is created without an id on the embed code, Video.js automatically assigns it one based on an auto-incrementing number (a.k.a. a GUID). For the longest time, this has happened to result in the default id of the first player being vjs_video_3. It was never intended for users to rely on this value being consistent, but users do strange and inadvisable things. PR #6103 had an unintended side effect in that it changed the default id to vjs_video_2, which we worry could affect some users of Video.js.
|
Rollup did't compiler the WeapMap, as a result, there is an error "WeapMap is undefined" in Android 4.x |
|
@wind-stone thanks for bringing it up. We'll work on figuring out how to make it better. |
|
I have a PR to fix this pending here #6289 |
|
We merged #6289 and will be releasing it soon. |
…me people have relied on this (#6216) When a player is created without an id on the embed code, Video.js automatically assigns it one based on an auto-incrementing number (a.k.a. a GUID). For the longest time, this has happened to result in the default id of the first player being vjs_video_3. It was never intended for users to rely on this value being consistent, but users do strange and inadvisable things. PR #6103 had an unintended side effect in that it changed the default id to vjs_video_2, which we worry could affect some users of Video.js.
Description
Use a weakmap so that we don't have to do the listener removal dance anymore on dispose.