fix(web): allows registering precached keyboards #9304
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
| this.cache.addStub(entry); | ||
| }); | ||
| } | ||
| } finally { } |
There was a problem hiding this comment.
Why is this empty finally{} needed?
There was a problem hiding this comment.
Are naked try { } statements permitted in JS?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch
The
try...catchstatement is comprised of atryblock and either acatchblock, afinallyblock, or both.
I want to let errors pass through.
keyman.registeritself is an undocumented API function.- Since there's no lead-in Promise, type-checking all possible input variants is... less than straightforward. We can at least be confident for the keymanweb.com use case, though.
There was a problem hiding this comment.
What is a naked try{} supposed to do though? It doesn't do anything, does it?
There was a problem hiding this comment.
Yeah, this was probably end-of-day brain. It does signal that we acknowledge the potential for errors, but I guess a comment can do that too.
There was a problem hiding this comment.
Fixed via code-suggestion in the (resolved) comment-chain below.
| // Handles keymanweb.com's precached keyboard array. There is no associated promise, | ||
| // so there's nothing handling the `register` call's results otherwise. | ||
| this.cloudQueryEngine.on('unboundregister', (registration) => { | ||
| // Internal, undocumented use-case of `keyman.register`: precached keyboard loading | ||
| // Other uses may trigger errors, especially if there's a type-structure mismatch. | ||
| // Those errors should not be handled here; let them surface. | ||
| if(Array.isArray(registration)) { | ||
| registration.forEach((entry) => { | ||
| this.cache.addStub(entry); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Given keyman.register was completely undocumented (which is really not a good thing given it definitely is an internal API), it's a bit of a stretch to say this was an undocumented use-case: all use-cases were undocumented.
It's perhaps something you did not consider when you changed the implementation to use promises. This issue underlines just how critical it is to avoid changing APIs, even internal undocumented ones, without full audit of all uses.
There was a problem hiding this comment.
Yeah. It does help to know such uses exist, though - took a bit to connect the dots once I realized keymanweb.com's caching might be related to the reported issue, especially since nothing else related was broken.
There was a problem hiding this comment.
That's the audit bit -- no one is going to remember everything, so code search is our friend here. For web, it's particularly troublesome given the websites can also interact. And 'register' is an unhelpfully common term so a search for this would have been particularly painful!
But in any case, searching api.keyman.com, keymanweb.com, help.keyman.com, and keyman.com sources any time any 'public' interface is touched on KeymanWeb will probably help with this in future. (All those sites use KeymanWeb or interact with KeymanWeb in some way or other, e.g. bookmarklet, embedded documentation OSK, etc.)
waaah 😭 |
|
Changes in this pull request will be available for download in Keyman version 17.0.145-alpha |
Fixes keymanapp/keymanweb.com#77.
The modularized design for KMW's cloud interactions assumes that there's always a
Promisefor any incomingkeyman.registercalls. That said, keymanweb.com likes to pre-cache the keyboards query and directly register it in a manner that bypasses cloud queries and the associated Promises. Since there was noPromisebound to the register, KMW had no infrastructure for forwarding the precached stubs.So, this PR adds in a relatively simple rigging to remedy that issue.
@keymanapp-test-bot skip