Correctly handle the event.currentTarget property #4559
Conversation
|
The generated output of |
|
Crap, I forgot that this would change the generated types. I think based on that alone we'll need a compat flag here... ugh. |
|
I don't think changing the generated types in itself is a reason for a compat flag -- the types don't exist in production, after all, and compat flags are about protecting workers in production. But whether the API change itself requires a compat flag, I don't know. We could add logging to see if anyone actually calls this when Though that doesn't really answer whether the undefined->null change in itself might require a compat flag, which I guess is harder to tell. |
|
I wonder if we should create a new compat flag called That way we don't create a massive number of compat flags for little things nobody actually cares about... cc @npaun |
|
That would make sense I think |
|
@kentonv Yeah I've considered creating one a few times now |
|
I'll move this back to draft, add a compat flag, and will bundle multiple WPT fixes under this PR. We can likely do this in batches with several compat flags, one for each batch. |
jasnell
force-pushed
the
jasnell/eventtarget-currenttarget-fix
branch
2 times, most recently
from
59d5a44 to
e1256e0
Compare
|
PR updated to add the compat flag and also handle the |
|
|
||
| pedanticWpt @101 :Bool | ||
| $compatEnableFlag("pedantic_wpt") | ||
| $compatDisableFlag("non_pedantic_wpt"); |
There was a problem hiding this comment.
Review note: this intentionally has no disable enable date for now.
| @@ -45,9 +45,7 @@ export default { | |||
| 'EventTarget-addEventListener.any.js': {}, | |||
| 'EventTarget-constructible.any.js': { | |||
| comment: 'Should be null, not EventTarget', | |||
| expectedFailures: [ | |||
There was a problem hiding this comment.
Shouldn't there be a change relating to isTrusted here as well?
There was a problem hiding this comment.
I thought so too but it passed locally without it.. so... I was confused lol... wanted to see what the main CI did.
Per the spec, the currentTarget property of an Event should be null if the event is not being dispatched. Introduces a `pedantic_wpt` compat flag for correctly handling these.
jasnell
force-pushed
the
jasnell/eventtarget-currenttarget-fix
branch
from
7285b2a to
d61e1ed
Compare
Per the spec, the currentTarget property of an Event should be null if the event is not being dispatched.
Fixes: #4558
Fixes: #4556
@danlapid @kentonv ... I'm hoping this can be something we don't need a compat flag for. Whatcha think?Adds a new
pedantic_wptcompat flag for these kinds of fixes. Additional changes will be made incrementally usingimplied_bycompat flags so that'll bundle all these types of changes together./cc @npaun