-
-
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): updates TypeScript version to 5.4.5 #11414
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
5819c42
to
ecd68fb
Compare
Current build failure is from commit 1, which reconnected tests that really should've been included as part of the prior PR. I just hadn't realized they were disconnected at the time. (The commit will likely disappear before this goes out of draft.) |
ecd68fb
to
408fe43
Compare
0bd13ee
to
1207c37
Compare
… from main configs
408fe43
to
d70463a
Compare
}, | ||
"devDependencies": { | ||
"@types/node": "^18.7.18" | ||
} |
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.
I wish there was a way in package.json to clearly document dependencies for a package while hoisting the version requirement to the top level. It's useful documentation to have 😭
"module": "ES2022", | ||
"module": "node16", |
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 a scary config change! Are you confident that it isn't going to break bundling, etc, for kmc, developer/server, etc?
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.
Yes. This is the prescribed "correct" way to do things in TS 5; we cannot leave ES2022
in place, as it's now a TS config error to do so. Use of "moduleResolution": "node16"
explicitly requires use of "module": "node16"
. They must match. We cannot update to TS 5.0+ without doing this.
By the way, when I previously alluded to things breaking when trying TS 5.0 in the past, this was the primary reason. Note: we aren't changing the "target"
value - just the "module"
value. target
defines the ES version, while module
defines the package style + package-resolution semantics. (I'll admit, it did take me a while to wrap my head around the new paradigm they established.)
Would you like me to add a user test that verifies that the resulting Developer build is unaffected? If so, would you prefer it here, on the final related PR (#11464), or both?
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.
Okay, that makes sense, thanks.
It'd be good for the user test to run on the final PR in the chain. It should be fine, as far as I can tell, but I am getting increasingly wary with config changes and unanticipated outcomes in js/ts world, particularly where we have so many modules with varying histories.
I think we need user tests of the compile toolchain (compiling within Keyman Developer IDE is fine) and Keyman Developer Server (so testing web keyboards in Keyman Developer IDE).
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.
FWIW, the Developer build from this PR (which passed) seems to have a fully-functioning Web test-host page when I test it locally myself. Recompilation of the keyboard (after a .keyman-touch-layout edit) seems to proceed fine.
That said... I noticed something really strange. If I edited the touch-layout a bit, then built the keyboard and tested via Developer Server, all was fine. If I then returned to the touch-layout editor and used CTRL-Z
(undo) once, saved, then rebuilt the keyboard and resumed testing... the results did not get updated. I was able to determine that the touch-layout (undo) edits did not make it into the compiled .js in this case.
Re-editing the touch layout, then copying-and-pasting the original value for edited parts of the touch layout back in place, then doing the usual save -> rebuild -> test, acted as expected. The test-host keyboard was properly responsive to updates after doing so. Looks to be something undo-specific?
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.
Looks to be something undo-specific?
Possibly yes.
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.
Forgot to link it previously within this comment-chain: I spun that "something really strange" observation off into #11514.
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
3 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 |
Changes in this pull request will be available for download in Keyman version 18.0.45-alpha |
Fixes #11375.
Also updates @types/node to a consistent version across all packages, as it caused a conflict after the TS upgrade. I'd left it with partly 18.0 and partly 20.0 in #11319, and it looks like our hand is forced with the TS upgrade.
@keymanapp-test-bot skip