Skip to content
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

Merged
merged 10 commits into from
Jun 3, 2024

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented May 16, 2024

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

@mcdurdin
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this added?

Copy link
Contributor Author

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:

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:

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... 🤷

@@ -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']) {
Copy link
Member

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?

Copy link
Contributor Author

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";
Copy link
Member

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.

@mcdurdin mcdurdin modified the milestones: A18S2, A18S3 May 24, 2024
@jahorton jahorton marked this pull request as draft May 28, 2024 02:39
@jahorton jahorton force-pushed the chore/web/remove-unused-locals branch 3 times, most recently from 82db2c6 to 50a194a Compare May 29, 2024 02:24
@jahorton jahorton force-pushed the chore/web/remove-no-implicit-any branch from 6209a4f to 4e80e63 Compare May 29, 2024 03:44
@jahorton jahorton changed the base branch from chore/web/remove-unused-locals to chore/web/partial-config-cleanup May 29, 2024 03:44
@jahorton jahorton marked this pull request as ready for review May 29, 2024 03:45
@jahorton jahorton changed the title chore(web): fixes cases of implicit-any in web/ chore(web): fixes cases of implicit-any in web/ 🔩 May 29, 2024
@jahorton jahorton marked this pull request as draft May 29, 2024 03:52
@@ -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,
Copy link
Contributor Author

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.

@jahorton jahorton marked this pull request as ready for review May 29, 2024 03:56
Co-authored-by: Marc Durdin <marc@durdin.net>
@jahorton jahorton changed the title chore(web): fixes cases of implicit-any in web/ 🔩 chore(web): correct cases of implicit-any in web/ 🔩 Jun 3, 2024
Base automatically changed from chore/web/partial-config-cleanup to master June 3, 2024 08:16
@jahorton jahorton merged commit f5e65f0 into master Jun 3, 2024
19 checks passed
@jahorton jahorton deleted the chore/web/remove-no-implicit-any branch June 3, 2024 08:16
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.47-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants