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

change(common/web): keyboard processor package modularization 🧩 #7809

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Nov 24, 2022

Continuing from #7800, this PR helps address #7309 by fully converting our internal @keymanapp/keyboard-processor package (and the testing package @keymanapp/recorder-core) to ES6 modules - including the former's unit tests!

Setting reviews to "no whitespace-only changes" is highly advised. As of 7849377: image

(Namespaces were generally the top-level indent; dropping them means removing the indent, hence the huge line-count difference.)

Note that this change removes the com.keyman.text.Codes access point for the Codes object; conversion to ES6 modules does mean that we'll be dropping the namespaces, hence no more com. After a bit of thought, I figured the ideal spot to move it would be as a property of KeyboardInterface, a global object (during operational KeymanWeb) that we must continue to support. Since even compiled keyboards still reference that... may as well add an access point to it and redirect debug keyboards there for the Codes object.

  • This may necessitate a corresponding change in Developer; note the altered testing keyboards for a non-minified example of how to handle said scenario.

Took a while to get the unit tests working; by default, ES module compilation doesn't actually care to ensure a smooth Node-based execution experience. Fortunately, telling tsc to use Node's module resolution and relying on node_modules-based paths for scripts from linked projects seems to do the trick - both the esbuild bundler and node execution for unit tests are happy this way.

Finally, the new index.ts file may be worth a look - it publishes the 'old' namespace-style organization of keyboard-processor and writes it to the global object... so in theory, with a bit more work, we may be able to link higher-level namespaced code with the modularized version seen here.

Now, whether or not that - putting energy into a hybrid state - is worth the effort... is a different question. I'm not sure how well it'd work with the namespaced projects' setup at the moment. That said, I'm also not sure how easy it'll be to properly modularize the top-level KMW code... some of that source has interesting cross-effects across files, which may need serious cleanup before modularizing. I'm honestly more afraid of the latter than the former.

@keymanapp-test-bot skip

@jahorton jahorton added this to the 17.0 milestone Nov 24, 2022
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Nov 24, 2022
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Nov 24, 2022

User Test Results

Test specification and instructions

User tests are not required

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Nov 24, 2022
@github-actions github-actions bot added web/ and removed web/ labels Nov 25, 2022
@jahorton jahorton force-pushed the change/web/util-modularization branch from 24a46bf to 1966468 Compare January 20, 2023 06:03
@github-actions github-actions bot added web/ and removed web/ labels Jan 31, 2023
@jahorton
Copy link
Contributor Author

jahorton commented Jan 31, 2023

OK... after a bit of work, I've figured out how to use tsc to handle declaration emissions. This works fantastically for development within the repository, but there are issues for supplying declarations to external consumers. (These are still far less notable than the limitations & issues I ran into with the dts-bundle-generator package.)

So, the point of concern, from a tsc-based declaration-only build:

declare module "text/keyboardProcessor" {
    import type Keyboard from "keyboards/keyboard";
    import KeyEvent from "text/keyEvent";
    import type { MutableSystemStore } from "text/systemStores";
    import type OutputTarget from "text/outputTarget";
    import KeyboardInterface, { VariableStore } from "text/kbdInterface";
    import RuleBehavior from "text/ruleBehavior";
    import { DeviceSpec } from "@keymanapp/web-utils/build/obj/index.js";

    // ...
}

Note that final import statement: tsc will not bundle in types from other repo-local packages that are considered external to the TS project itself. Just to be clear - that's the build output for a package's / project's "bundled" declaration file, with all modules' declarations bundled into a single output file. tsc simply will not include declarations for imported modules not within the project. As per microsoft/TypeScript#4433 (comment), this seems to be a limitation caused by relying upon compilerOptions.paths in the TS config.

That said, compilerOptions.paths is quite convenient for keeping everything properly separated and modularized during development, and it helps facilitate our incremental build setup. So, I sought out possible ways forward that left the current compilation patterns intact... and landed on something.

Observations:

declare module "@keymanapp/web-utils/build/obj/index.js" { /* ... */ }

Adding the above block will redirect all import references to this module declaration. That said, unfortunately you can't nest another declare module block within that.

But!

<reference path="some-declaration-file.d.ts"/>

Such statements are totally legal within declaration files and can be used to 'attach' one to another, sort of like a pseudo-import for the referenced file's declarations.

So, the path forward:

  1. Find all "@keymanapp/..." import declarations and duplicate their bundled (lib/) declaration files next to the main build product's main .d.ts file.
  2. In each local .d.ts file, rename the final declare module block to reflect its original location, as with the "@keymanapp/web-utils/build/obj/index.js" example above.
  3. For each reference to such a .d.ts file, insert <reference path> tags at the top of each .d.ts file so that the cross-declaration-file imports may resolve.

Limitations:

  1. If concatenating all .d.ts files into one (as <reference path> effectively does), note that each declare module "___" name probably needs to be unique across all projects.
    • Fortunately, the primary not-unique example would be the top-level "index" module, which gets renamed in step 2 above.
    • Even that is only duplicated at the top level of each Web subproject.
  2. This may require knowledge of the obj vs lib build product organization scheme when converting obj-based imports to their lib-based declaration after esbuild-bundling occurs.

While not optimal, it's at least reasonably viable without being too fiddly. Note that we can also simply... not bother with this for now; the declarations work flawlessly for local development on components (or all of) Keyman Engine for Web. It only matters if and when we publish components / the engine itself to npm and/or external developers request type declaration files.

@mcdurdin
Copy link
Member

mcdurdin commented Feb 2, 2023

Note that this change removes the com.keyman.text.Codes access point for the Codes object; conversion to ES6 modules does mean that we'll be dropping the namespaces, hence no more com. After a bit of thought, I figured the ideal spot to move it would be as a property of KeyboardInterface, a global object (during operational KeymanWeb) that we must continue to support. Since even compiled keyboards still reference that... may as well add an access point to it and redirect debug keyboards there for the Codes object.

Ideally, we move Codes to common/web/types -- it's a set of constants that is useful across platforms.

  • This may necessitate a corresponding change in Developer; note the altered testing keyboards for a non-minified example of how to handle said scenario.

Not clear what you are saying here -- are you saying we need to change the compiler to emit something different for debug keyboards? Thus, debug keyboards built for Keyman 17+ will have different references to debug keyboards built for earlier versions?

While not optimal, it's at least reasonably viable without being too fiddly. Note that we can also simply... not bother with this for now; the declarations work flawlessly for local development on components (or all of) Keyman Engine for Web. It only matters if and when we publish components / the engine itself to npm and/or external developers request type declaration files.

I think we don't bother with this for now. We're not doing npm publishing of KMW at this point, so we can cross this bridge at the time that we do.

@jahorton
Copy link
Contributor Author

jahorton commented Feb 2, 2023

Note that this change removes the com.keyman.text.Codes access point for the Codes object; conversion to ES6 modules does mean that we'll be dropping the namespaces, hence no more com. After a bit of thought, I figured the ideal spot to move it would be as a property of KeyboardInterface, a global object (during operational KeymanWeb) that we must continue to support. Since even compiled keyboards still reference that... may as well add an access point to it and redirect debug keyboards there for the Codes object.

Ideally, we move Codes to common/web/types -- it's a set of constants that is useful across platforms.

I've come to that conclusion, too.

While not optimal, it's at least reasonably viable without being too fiddly. Note that we can also simply... not bother with this for now; the declarations work flawlessly for local development on components (or all of) Keyman Engine for Web. It only matters if and when we publish components / the engine itself to npm and/or external developers request type declaration files.

I think we don't bother with this for now. We're not doing npm publishing of KMW at this point, so we can cross this bridge at the time that we do.

Yep. Maybe I didn't imply it strongly enough there?

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.

Only minor things, I think

common/test/resources/keyboards/khmer_angkor.js Outdated Show resolved Hide resolved
common/test/resources/keyboards/test_deadkeys.js Outdated Show resolved Hide resolved
common/test/resources/keyboards/web_context_tests.js Outdated Show resolved Hide resolved
Comment on lines +6 to +7
export { default as Codes } from "./text/codes.js";
export * from "./text/codes.js";
Copy link
Member

Choose a reason for hiding this comment

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

I suggest that we move codes.ts into common/web/types as a future refactor.

common/web/keyboard-processor/src/text/codes.ts Outdated Show resolved Hide resolved
common/web/keyboard-processor/src/text/codes.ts Outdated Show resolved Hide resolved
Comment on lines +185 to +190
export enum SystemStoreIDs {
TSS_LAYER = 33,
TSS_PLATFORM = 31,
TSS_NEWLAYER = 42,
TSS_OLDLAYER = 43
}
Copy link
Member

Choose a reason for hiding this comment

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

This ideally belongs in common/web/types longer term

Comment on lines +5 to +7
import KeyboardProcessor from '@keymanapp/keyboard-processor/build/obj/text/keyboardProcessor.js';
import { KeyboardTest } from '@keymanapp/recorder-core/build/obj/index.js';
import NodeProctor from '@keymanapp/recorder-core/build/obj/nodeProctor.js';
Copy link
Member

Choose a reason for hiding this comment

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

It'd be good to be able to reduce this to:

Suggested change
import KeyboardProcessor from '@keymanapp/keyboard-processor/build/obj/text/keyboardProcessor.js';
import { KeyboardTest } from '@keymanapp/recorder-core/build/obj/index.js';
import NodeProctor from '@keymanapp/recorder-core/build/obj/nodeProctor.js';
import KeyboardProcessor from '@keymanapp/keyboard-processor';
import NodeProctor, { KeyboardTest } from '@keymanapp/recorder-core';

Co-authored-by: Marc Durdin <marc@durdin.net>
@github-actions github-actions bot added web/ and removed web/ labels Feb 2, 2023
@github-actions github-actions bot added web/ and removed web/ labels Feb 2, 2023
@github-actions github-actions bot added web/ and removed web/ labels Feb 2, 2023
@github-actions github-actions bot added web/ and removed web/ labels Feb 2, 2023
Base automatically changed from change/web/util-modularization to feature-esmodule-web-engine February 2, 2023 07:07
@jahorton jahorton merged commit caa4165 into feature-esmodule-web-engine Feb 2, 2023
@jahorton jahorton deleted the change/common/web/keyboard-processor-modularization branch February 2, 2023 07:07
@mcdurdin mcdurdin modified the milestones: 17.0, A17S19 Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants