-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
chore(web): correct cases of implicit-any in web/ 🔩 #11462
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
Can you include a reference for why this is being done? |
@@ -23,6 +23,7 @@ export class TextTransform implements Transform { | |||
readonly deleteLeft: number; | |||
readonly deleteRight: number; | |||
readonly erasedSelection: boolean; | |||
id: number; |
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.
why is this added?
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.
Brought to you by noImplicitAny
. TS began complaining about its type not being available, as it is referenced in some of the predictive-text integration code.
On the base type Transform
, id
is marked as optional:
keyman/common/models/types/index.d.ts
Lines 226 to 234 in 963587f
declare interface Transform { | |
/** | |
* Facilitates use of unique identifiers for tracking the Transform and | |
* any related data from its original source, as the reference cannot be | |
* preserved across WebWorker boundaries. | |
* | |
* This is *separate* from any LMLayer-internal identification values. | |
*/ | |
id?: number; |
Within this type (TextTransform
), it was not declared at all:
keyman/common/web/keyboard-processor/src/text/outputTarget.ts
Lines 21 to 26 in 963587f
export class TextTransform implements Transform { | |
readonly insert: string; | |
readonly deleteLeft: number; | |
readonly deleteRight: number; | |
readonly erasedSelection: boolean; | |
As a result, TS inferred that TextTransform
simply didn't support .id
. I'm not 100% sure why noImplicitAny
was the trigger, because it previously did infer type, but... 🤷
web/src/app/ui/kmwuibutton.ts
Outdated
@@ -293,7 +293,7 @@ if(!keyman?.ui?.name) { | |||
|
|||
/* TODO: why is this still needed??? Does it actually do anything?? */ | |||
osk.addEventListener('hide', function(obj) { | |||
if((arguments.length > 0) && obj['hiddenByUser']) { | |||
if((arguments.length > 0) && obj['HiddenByUser']) { |
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.
Is this a bug fix?
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.
Brought to you by noImplicitAny
. It could detect typings after the fix, but not before.
@@ -3,7 +3,5 @@ export default function createUnselectableElement<E extends "p"|"style"|"script" | |||
const e = document.createElement<E>(nodeName); | |||
|
|||
e.style.userSelect="none"; | |||
e.style.webkitUserSelect="none"; | |||
e.style['MozUserSelect'] = "none"; |
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.
This is probably okay as it Firefox tends to be evergreen.
82db2c6
to
50a194a
Compare
6209a4f
to
4e80e63
Compare
@@ -11,9 +11,6 @@ | |||
// but existing code in web/ breaks some of these settings | |||
// | |||
// At present, the code actually compiles without them... but the TS tests are a different matter. | |||
"noImplicitAny": false, | |||
"noUnusedLocals": false, |
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.
The "noUnusedLocals"
bit should've been removed earlier, but must have gotten mangled a bit in all the rebasing to reorder... everything else.
Co-authored-by: Marc Durdin <[email protected]>
…e/web/remove-no-implicit-any
…implicit-any' into chore/web/remove-no-implicit-any
Changes in this pull request will be available for download in Keyman version 18.0.47-alpha |
This one was a bit less straightforward than some of the previous TS compiler flag PRs - certain cases required some special setup. (Such as the mouse & touch event emulation for certain automated tests.)
Addresses part of #11473.
@keymanapp-test-bot skip