chore(web): fixes implicit-this cases 🔩 #11459
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
jahorton
changed the title
|
Can you include a reference for why this is being done? (It's part of a larger task, right?) |
web/src/tsconfig.dom.json
Outdated
| // TODO: These override ../tsconfig.base.json settings, and so should be removed if possible, | ||
| // but existing code in web/ breaks some of these settings | ||
| "noImplicitAny": false, | ||
| "noImplicitThis": false, |
There was a problem hiding this comment.
Why this is being done: the // TODO: above.
| if(osk.vkbd.menuEvent) { | ||
| this.highlightKey(osk.vkbd.menuEvent, false); | ||
| } | ||
| if(typeof(engine.menuKeyUp) == 'function') { // VisualKeyboard event: globeKey | ||
| engine.menuKeyUp(); | ||
| } | ||
| osk.vkbd.menuEvent = null; |
There was a problem hiding this comment.
This change doesn't seem to belong? Seems like a difference in behaviour.
There was a problem hiding this comment.
See line 26. It's not immediately obvious, but osk.vkbd.menuEvent would hold the same value as key for this event. We were de-highlighting the key twice.
Secondly, note that there was no reasonable this that actually had a highlightKey function; that in itself needed correcting. This should have triggered null or undefined-reference errors, yet we've never seen anything arise from this block of code. The code path is actually dead, but I felt removing the whole thing would be an even bigger change.
| var event = Pelem as Event; | ||
|
|
||
| if(this.instanceof(event.target, 'Window')) { | ||
| scopedClass = event.target[className]; | ||
| } else if(this.instanceof(event.target, 'Document')) { | ||
| scopedClass = (event.target as Document).defaultView[className]; | ||
| } else if(this.instanceof(event.target, 'HTMLElement')) { | ||
| scopedClass = (event.target as HTMLElement).ownerDocument.defaultView[className]; | ||
| } | ||
| const event = Pelem as Event; | ||
| return nestedInstanceOf(event.target, className); |
There was a problem hiding this comment.
This also looks like a behavioural change?
There was a problem hiding this comment.
Another case of "there's no reasonable this supporting the specified method." Turns out, we have no cases of calling nestedInstanceOf with an event object; we'd have been getting null/undefined errors otherwise. The code path is actually dead at the moment.
That said, were we to change paths on this fact in the future, the easiest approach is to simply get the event target and act as if that was the original parameter. Compare with lines 20-25 - they're checking for the same things once we substitute event.target in for Pelem.
jahorton
force-pushed
the
chore/web/fix-implicit-this
branch
from
eef8c32 to
a5c10e8
Compare
jahorton
changed the base branch from
chore/web/partial-config-cleanup
to
fix/web/remove-dead-branches
jahorton
changed the title
|
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
|
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
2 similar comments
|
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
|
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
Addresses part of #11473, correcting all remaining cases caught by enforcing `"noImplicitThis" across all Keyman Engine for Web related code.
@keymanapp-test-bot skip