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.

Sign up for GitHub

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

Jump to bottom

feat(web): refactor loading of .js keyboards 🎼 #12334

Merged
merged 10 commits into from
Oct 9, 2024
Merged

feat(web): refactor loading of .js keyboards 🎼 #12334

merged 10 commits into from
Oct 9, 2024

Conversation

ermshiperete
Copy link
Contributor

  • split keyboard loading into loading into blob and then loading the script
  • look at first four bytes to see if it's a .js or a .kmx keyboard
  • for domKeyboardLoader, use fetch to get the blob, then use indirect eval to load the script, instead of injecting a script element.

This does not yet implement the loading of .kmx keyboards.

@keymanapp-test-bot skip

@ermshiperete
feat(web): refactor loading of .js keyboards
f7f26e8 
- split keyboard loading into loading into blob and then loading the script
- look at first four bytes to see if it's a .js or a .kmx keyboard
- for domKeyboardLoader, use fetch to get the blob, then use indirect
  eval to load the script, instead of injecting a script element.

This does not yet implement the loading of .kmx keyboards.
@ermshiperete ermshiperete requested a review from jahorton as a code owner August 31, 2024 14:59
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Aug 31, 2024 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot changed the title feat(web): refactor loading of .js keyboards feat(web): refactor loading of .js keyboards 🎼 Aug 31, 2024
@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S10 milestone Aug 31, 2024
mcdurdin
mcdurdin requested changes Sep 10, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

This is looking good, most of my commentary is around error management. It would be good to be able to unit test the various error scenarios.

@ermshiperete
chore(web): allow to run unit tests in vscode test explorer
cbbe971 
#11746 removed the `ts-node` dependency. Unfortunately that broke running
TypeScript test files with Mocha in VSCode Test Explorer because it no
longer knows what to do with .ts files. This change adds the more modern
`tsx` package as developer dependency, which allows to use Test Explorer
again.

See https://stackoverflow.com/a/77609121.
@ermshiperete
feat(web): add error handling and tests
1dd6989 
Addresses code review comments.
@ermshiperete
Copy link
Contributor Author

Note that this includes #12400 now (cbbe971).

1dd6989 adds error handling and also adds two unit tests. Note that I moved keyboard-loading.js test file to keyboard-loading.tests.ts.

mcdurdin
mcdurdin reviewed Sep 13, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

I like where this is going. Have a number of additional comments, but it's getting close. Given this is so core to KMW, I think it is important to get it really solid!

@darcywong00 darcywong00 modified the milestones: A18S10, A18S11 Sep 14, 2024
@ermshiperete
feat(web): improve errors and tests
94b06b7 
Addresses code review comments.
@jahorton
Copy link
Contributor

Make sure to test & user-test with the page found at /web/src/test/manual/web/keyboard-errors/ if you aren't already doing so; it's designed to test keyboard-load error handling.

@ermshiperete
Copy link
Contributor Author

ermshiperete commented Sep 24, 2024 •
edited
Loading

Make sure to test & user-test with the page found at /web/src/test/manual/web/keyboard-errors/ if you aren't already doing so; it's designed to test keyboard-load error handling.

@jahorton Thanks for the hint! That showed two scenarios that this PR doesn't handle correctly. One (non-parsable) is easy to fix, but for the other (timeout) I'm not sure how to make it behave the same as before: for that test web/src/test/manual/web/keyboard-errors/timeout.js accesses document.currentScript to prevent onload from triggering. However, since we now use eval document.currentScript doesn't get set. Any ideas how to fix this?

@jahorton
Copy link
Contributor

Make sure to test & user-test with the page found at /web/src/test/manual/web/keyboard-errors/ if you aren't already doing so; it's designed to test keyboard-load error handling.

@jahorton Thanks for the hint! That showed two scenarios that this PR doesn't handle correctly. One (non-parsable) is easy to fix, but for the other (timeout) I'm not sure how to make it behave the same as before: for that test web/src/test/manual/web/keyboard-errors/timeout.js accesses document.currentScript to prevent onload from triggering. However, since we now use eval document.currentScript doesn't get set. Any ideas how to fix this?

Since we're using await fetch to load the keyboard now...

  • if there's an error that fetch can throw if it takes a long time, use that to generate it
  • We could use Promise.race on both the fetch's Promise and a timedPromise(10000). When it's the latter that is fulfilled, have it throw the timeout error.

@mcdurdin
Copy link
Member

Make sure to test & user-test with the page found at /web/src/test/manual/web/keyboard-errors/ if you aren't already doing so; it's designed to test keyboard-load error handling.

@jahorton Thanks for the hint! That showed two scenarios that this PR doesn't handle correctly. One (non-parsable) is easy to fix, but for the other (timeout) I'm not sure how to make it behave the same as before: for that test web/src/test/manual/web/keyboard-errors/timeout.js accesses document.currentScript to prevent onload from triggering. However, since we now use eval document.currentScript doesn't get set. Any ideas how to fix this?

Since we're using await fetch to load the keyboard now...

  • if there's an error that fetch can throw if it takes a long time, use that to generate it
  • We could use Promise.race on both the fetch's Promise and a timedPromise(10000). When it's the latter that is fulfilled, have it throw the timeout error.

https://stackoverflow.com/a/50101022/1836776 discusses ways to handle timeout for Fetch -- and discourages use of Promise.race.

mcdurdin
mcdurdin requested changes Sep 25, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

The updates look good, but I realized there is still one problem, with .kmx detection. Only a couple of other minor nits.

@darcywong00 darcywong00 modified the milestones: A18S11, A18S12 Sep 28, 2024
@ermshiperete
feat(web): re-implement timeout error test
c05cf0e 
The previous approach with overriding `onload` no longer works if we use
indirect eval. However, since we now need to use a server anyways instead
of directly loading a file (because of Core), we can add a timeout to
the test server and implement this test in a cleaner way.
Base automatically changed from chore/merge-master-into-web-core to epic/web-core October 9, 2024 07:01
@ermshiperete ermshiperete merged commit 73c0775 into epic/web-core Oct 9, 2024
16 of 17 checks passed
@ermshiperete ermshiperete deleted the feat/web/readkbd branch October 9, 2024 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

@jahorton jahorton Awaiting requested review from jahorton jahorton is a code owner

@rc-swag rc-swag Awaiting requested review from rc-swag

@sgschantz sgschantz Awaiting requested review from sgschantz

@SabineSIL SabineSIL Awaiting requested review from SabineSIL

@darcywong00 darcywong00 Awaiting requested review from darcywong00

Assignees
No one assigned
Labels
android/app/ android/engine/ android/ common/resources/ Build infrastructure common/web/ common/ developer/compilers/ developer/ide/ developer/ docs epic-web-core feat ios/ linux/config/ linux/ mac/ oem/fv/ oem/ web/
Projects
Keyman
Archived in project
Milestone
A18S12
Development

Successfully merging this pull request may close these issues.
4 participants
@ermshiperete @jahorton @mcdurdin @darcywong00