change(web): remove support for es5 #11881

ermshiperete · 2024-06-26T14:40:13Z

Fixes: #11878

@keymanapp-test-bot skip

keymanapp-test-bot · 2024-06-26T14:40:16Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 18.0.66 (0.11881.11413)
Keyboards
- Test Keyboards
Web
- KeymanWeb Test Home
Windows

Fixes: #11878

…'s version Node's version has special behavior for events named 'error' - if no handler, it auto-throws. EventEmitter3 does not include this.

This fixes building all dependencies, e.g. when running `./build.sh test` which previously failed because it didn't build all necessary targets.

jahorton · 2024-07-05T01:26:39Z

common/web/keyboard-processor/build.sh

@@ -45,21 +45,26 @@ function do_build() {
  tsc --build "$THIS_SCRIPT_PATH/tsconfig.all.json"

  # Base product - the main keyboard processor
-  $BUNDLE_CMD    "${KEYMAN_ROOT}/common/web/keyboard-processor/build/obj/index.js" \
+  ${BUNDLE_CMD}    "${KEYMAN_ROOT}/common/web/keyboard-processor/build/obj/src/index.js" \


Why the change in path here?

Also, just to be clear - be very careful when changing the build-output paths like this - it has a tendency to have unwanted side effects in the end-result sourcemap files, often causing the pathing structure shown when debugging via Chrome to become very messy and thus more difficult to utilize.

Part of the reason the /src/ bit wasn't in there to begin with is that omitting it allows the compiled *.js files to be at the same depth as their source counterparts, and that seriously simplifies the sourcemap-editing operations that we apply during top-level compilation. (I forget some of the details; the main reason it was so helpful might be fully in the past, but there's a chance it could be something that would still apply even now.)

jahorton · 2024-07-05T01:32:58Z

common/web/keyboard-processor/tests/dom/cases/domKeyboardLoader.spec.ts

+    if(window['KeymanWeb' as any]) {
+      (window['KeymanWeb' as any] as any).uninstall();


Try using this at the file's top:

declare let KeymanWeb: KeyboardInterface

It's possible to declare globals with type, after which you won't need the as any stuff.

We already use this pattern in a few places, such as the spot below:

keyman/web/src/app/ui/kmwuitoggle.ts

Line 9 in b15dcef

declare var keyman: KeymanEngine

With that in place, it doesn't fix the window['keyman'] pattern, but it does allow us to just assume that keyman exists as a global, which achieves what we're really after anyway.

jahorton · 2024-07-05T01:38:33Z

common/web/keyboard-processor/tests/dom/web-test-runner.config.mjs

@@ -21,7 +21,7 @@ export default {
  concurrency: 10,
  nodeResolve: true,
  files: [
-    '**/*.spec.ts'
+    'build/lib/**/*.spec.mjs'


I would strongly prefer a specific build folder for tests, then a specific folder for each test set.

Something like build/test/dom/**/*.spec.mjs - this pattern is being maintained for /web/src/test/auto/dom/ tests, so why not be consistent with the pattern and reuse it here?

(While this module's headless tests aren't yet in TS, they could then go under build/test/headless/**.)

A general pattern is for lib folders to hold bundled / fully-compiled outputs, not testing files... right? Or am I just mistaken about that? Either way, that's a pattern I've held to strongly with the lower-level packages; test file outputs do not belong in that folder, in my opinion. Too messy.

jahorton · 2024-07-05T01:42:05Z

web/src/test/auto/dom/web-test-runner.config.mjs

-        'src/test/auto/dom/cases/attachment/**/*.spec.html',
-        'src/test/auto/dom/cases/attachment/**/*.spec.ts'
+        'build/test/dom/cases/attachment/**/*.spec.html',
+        'build/test/dom/cases/attachment/**/*.spec.mjs'


To compare with the comment I made for keyboard-processor tests: note how the build/test/dom/** pattern is being used here, with minimal changes to boot.

jahorton · 2024-07-05T01:44:24Z

common/web/lm-worker/build-polyfiller.js

Note that this file exists to polyfill for things that would be otherwise missing with ES5, etc. There's a strong chance we should be able to eliminate this script entirely, though it'd take a bit of extra work.

(To be clear, that "extra work" could be done later; just make a tracking issue if so.)

jahorton · 2024-07-05T01:47:05Z

common/web/tslib/README.md

Ooh, it would be nice to drop this if viable. I'll want to double-check the outputs to ensure that we aren't getting duplicated/triplicated tslib helper functions due to it, just to be sure, but the comments originally at the top here suggest that we should be fine.

The Web filesize-increase reporter should signal a significant spike in filesize if it's not resolving the helpers properly.

... Hmm. We went from +32 bytes from the prior PR to +4619 bytes in this one. It's definitely worth double-checking the build output for duplicated helpers, then.

jahorton · 2024-07-05T01:50:12Z

common/web/types/src/deps/xml2js/parser.js

+          if(this.listenerCount('error') > 0) {
+            this.emit('error', err);
+          } else {
+            throw err;
+          }


For other reviewers, just in case:

We use eventemitter3 so that Web can safely compile against common/web/types; events is a Node-specific package that won't exist in the browser. There is one particular point of divergence between the two types:

In events, .emit('error') will actually throw the error if there are no registered listeners.

In eventemitter3, this specialized behavior is omitted. It will never throw on .emit('error') regardless of listener count.

This is the one spot in xml2js that uses .emit('error'), so we opted for a precise adjustment to accommodate for the difference between the two implementations that will replicate the behavior of the original. One of the common/web/types unit tests fails otherwise due to reliance on the pattern.

jahorton · 2024-07-05T01:52:31Z

web/build.sh

+  configure                     "/node_modules" \
+  build                         "/web/build/test/dom/cases/attachment/outputTargetForElement.spec.html" \
+  build:app/browser             "/web/build/app/browser/lib/index.mjs" \
+  build:app/WebViews            "/web/build/app/webview/${config}/keymanweb-webview.js" \


Suggested change

build:app/WebViews "/web/build/app/webview/${config}/keymanweb-webview.js" \

build:app/webview "/web/build/app/webview/${config}/keymanweb-webview.js" \

keymanapp-test-bot bot added this to the A18S5 milestone Jun 26, 2024

github-actions bot added web/ common/ common/web/ change labels Jun 26, 2024

ermshiperete force-pushed the change/web/11878_remove-es5 branch from b283ea1 to d4e63a4 Compare June 26, 2024 15:21

github-actions bot added android/ android/engine/ labels Jun 26, 2024

ermshiperete force-pushed the change/web/11878_remove-es5 branch from d4e63a4 to 5a80dbf Compare July 2, 2024 17:20

ermshiperete marked this pull request as ready for review July 2, 2024 17:20

ermshiperete requested review from jahorton, mcdurdin, rc-swag, darcywong00 and sgschantz as code owners July 2, 2024 17:20

ermshiperete changed the base branch from master to refactor/web/typescript July 2, 2024 17:21

ermshiperete marked this pull request as draft July 2, 2024 21:07

ermshiperete force-pushed the change/web/11878_remove-es5 branch 2 times, most recently from 68a1330 to a1bdce0 Compare July 4, 2024 16:52

ermshiperete and others added 3 commits July 4, 2024 20:49

change(web): remove support for es5

afe25ed

Fixes: #11878

fix(common): add workaround for divergence of EventEmitter3 from node…

877cd97

…'s version Node's version has special behavior for events named 'error' - if no handler, it auto-throws. EventEmitter3 does not include this.

fix(web): add dependency output

2d9d847

This fixes building all dependencies, e.g. when running `./build.sh test` which previously failed because it didn't build all necessary targets.

ermshiperete force-pushed the change/web/11878_remove-es5 branch 2 times, most recently from c2f69cc to 2d9d847 Compare July 4, 2024 20:21

ermshiperete marked this pull request as ready for review July 4, 2024 21:15

jahorton reviewed Jul 5, 2024

View reviewed changes

jahorton requested changes Jul 5, 2024

View reviewed changes

mcdurdin mentioned this pull request Jul 5, 2024

feat(web): check for low-probability exact + exact-key correction matches 📚 #11876

Open

darcywong00 removed this from the A18S5 milestone Jul 5, 2024

darcywong00 added this to the A18S6 milestone Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change(web): remove support for es5 #11881

change(web): remove support for es5 #11881

ermshiperete commented Jun 26, 2024

keymanapp-test-bot bot commented Jun 26, 2024 •

edited

Loading

jahorton Jul 5, 2024

jahorton Jul 5, 2024 •

edited

Loading

jahorton Jul 5, 2024

jahorton Jul 5, 2024

jahorton Jul 5, 2024

jahorton Jul 5, 2024 •

edited

Loading

jahorton Jul 5, 2024

jahorton Jul 5, 2024

jahorton Jul 5, 2024

jahorton Jul 5, 2024

		if(window['KeymanWeb' as any]) {
		(window['KeymanWeb' as any] as any).uninstall();

	build:app/WebViews "/web/build/app/webview/${config}/keymanweb-webview.js" \
	build:app/webview "/web/build/app/webview/${config}/keymanweb-webview.js" \

change(web): remove support for es5 #11881

Are you sure you want to change the base?

change(web): remove support for es5 #11881

Conversation

ermshiperete commented Jun 26, 2024

keymanapp-test-bot bot commented Jun 26, 2024 • edited Loading

User Test Results

Test Artifacts

Choose a reason for hiding this comment

jahorton Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahorton Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keymanapp-test-bot bot commented Jun 26, 2024 •

edited

Loading

jahorton Jul 5, 2024 •

edited

Loading

jahorton Jul 5, 2024 •

edited

Loading