refactor(core): control flows and types in event-dispatch #58752
Conversation
Clean some conditions and add missing types for more readability.
pullapprove
bot
added
the
requires: TGP
| copy['type'] = EventType.POINTERLEAVE; | ||
|
|
||
| switch (e.type) { | ||
| case EventType.MOUSEOVER: |
There was a problem hiding this comment.
this is more verbose than the previous implementation and has a high bundle size impact.
There was a problem hiding this comment.
Thanks for your feedback. I noticed that you didn't comment the first switch I implemented (at line n°22. in event.ts).
Was the change I made okay for you?
There was a problem hiding this comment.
We'll actually need to physically build this to check for size regressions before we can merge this.
thePunderWoman
requested review from
iteriani,
tbondwilkinson and
thePunderWoman
and removed request for
alxhub and
ellenyuan
…n some conditions and add missing types for more readability.
angular-robot
bot
added
area: core
| switch (eventType) { | ||
| case EventType.MOUSEENTER: | ||
| return EventType.MOUSEOVER; | ||
| case EventType.MOUSELEAVE: | ||
| return EventType.MOUSEOUT; | ||
| case EventType.POINTERENTER: | ||
| return EventType.POINTEROVER; | ||
| case EventType.POINTERLEAVE: | ||
| return EventType.POINTEROUT; | ||
| default: | ||
| return eventType; |
There was a problem hiding this comment.
Suggestion: instead of the if-else and the switch approaches why don't you go with a dictionary for object lookup? also it's to me better for maintenance (extension) and readability
| switch (eventType) { | |
| case EventType.MOUSEENTER: | |
| return EventType.MOUSEOVER; | |
| case EventType.MOUSELEAVE: | |
| return EventType.MOUSEOUT; | |
| case EventType.POINTERENTER: | |
| return EventType.POINTEROVER; | |
| case EventType.POINTERLEAVE: | |
| return EventType.POINTEROUT; | |
| default: | |
| return eventType; | |
| const eventMapping = { | |
| [EventType.MOUSEENTER]: EventType.MOUSEOVER, | |
| [EventType.MOUSELEAVE]: EventType.MOUSEOUT, | |
| [EventType.POINTERENTER]: EventType.POINTEROVER, | |
| [EventType.POINTERLEAVE]: EventType.POINTEROUT, | |
| }; | |
| return eventMapping[eventType] || eventType; |
There was a problem hiding this comment.
I made some tests on jsperf.app and jsbench.me with these three solutions (if, switch and dictionary).
The dictionary had good perfs but only if you define the eventMapping outside the function. In this case it could be at the end of the file with the other const, otherwise it may be up to 90% slower than the previous approach. What do you think @JeanMeche?
There was a problem hiding this comment.
Yes I wouldn't go with a map, as it introduces a lookup.
There was a problem hiding this comment.
Doing EventType.MOUSEENTER is also a lookup, since enums aren't actually a thing in JS and as far as I'm aware, enum values aren't inlined in the final bundle.
So the current code does actually do more lookups than than the object-map approach, since if you precompile the lookup-table as suggested, you are always doing one lookup, while the old code does always do at least two lookups and can do up to 5.
There was a problem hiding this comment.
Actually EventType isn't a typescript enum but a pure key value js object.
Is the lookup issue you're talking about still accurate in this case?
There was a problem hiding this comment.
Yes, it applies, it's basically what enum does under the hood. Sorry, I had checked EventType, which is also a type in the Angular router, where it actually is an enum and I didn't check if it's the correct one.
There was a problem hiding this comment.
What can be costly in javascsript it accessing an unknown key. Please have a look at this explaination.
| // `isContentEditable` is an old DOM API. | ||
| // tslint:disable-next-line:no-any | ||
| if ((el as any).isContentEditable) { | ||
| if ( |
There was a problem hiding this comment.
I'm not sure we could consider this a readability improvement.
Both before and after would minified to the same code, so there is no runtime improvement.
There was a problem hiding this comment.
Interesting, I made this change to return false in a single condition to avoid repetition but if you say that the minified code would be the same I'm okay to put back the previous conditions everywhere I did it.
|
If this were adding just types, this would be very safe to land, but since it has some changes to logic, there's some risk associated. This code is very size sensitive. So we'll close this for now. If you want to do something that just adds types, that should be quick to land. Thanks though! |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
5 participants
Clean some conditions and add missing types for more readability.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information