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(web): lm-worker bundling + wrapper script rework 📜 #10264

Merged
merged 12 commits into from
Jan 8, 2024

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Dec 15, 2023

This PR exists as prep-work (on the lm-worker level) toward having alternate compile targets, such as ES6 artifacts, from Web and Web-related builds. In particular, note the goal of #10257.

Notable differences within this PR:

  1. All "wrapping" functionality is handled by the build-wrapper.js script.
    • Sourcemaps are now always emitted by build-polyfiller.js; before, the adjustment was made here, rather than when wrapping.
    • Regardless of compile target, inclusion or exclusion of sourcemaps is now specified at the "wrapping" stage.
  2. All lm-worker .js build scripts are now parameterized, rather than having specific source and output files hardcoded.
    • This means that all paths are now specified and editable within the main build script, rather than in the helpers.
    • The resulting generalization will greatly facilitate code re-use for alternate compile targets.
    • I've given two of the three scripts a builder-like interface, including help documentation.
      • The excluded one? It gets overhauled in the next PR.

@keymanapp-test-bot skip

@jahorton jahorton requested a review from mcdurdin as a code owner December 15, 2023 04:52
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Dec 15, 2023

User Test Results

Test specification and instructions

User tests are not required

@github-actions github-actions bot added web/ and removed web/ labels Dec 20, 2023
@github-actions github-actions bot added web/ and removed web/ labels Dec 22, 2023
@mcdurdin mcdurdin modified the milestones: A17S28, A17S29 Dec 30, 2023
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.

LGTM, suggestions are only minor, I don't mind the wetness of the doHelp and parameter parsing at this point. Could consider using commander or some other command line parsing library but it hardly seems worth it at present.

common/web/lm-worker/build-polyfiller.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-polyfiller.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-polyfiller.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-polyfiller.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-polyfiller.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-polyfiller.js Outdated Show resolved Hide resolved
Comment on lines +189 to +190
// https://caniuse.com/mdn-javascript_builtins_function_name_configurable_true
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it might be flipped in 18.0? Do we need an issue to track this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming I'm properly understanding you here, we will not need an issue to track this. From #10257:

export function forES6(config: esbuild.BuildOptions): esbuild.BuildOptions {
return {
...config,
target: "es6",
conditions: ['es6-bundling'],
// Even if they'd be tree-shaken out, esbuild will fall over from their presence
// within an imported `@keymanapp/common-types`.
external: ['timers', 'buffer', 'events'],
// Available with ES6, but not necessarily with ES5.
keepNames: true
};
}

The ES6 target's configuration will re-enable this.

Copy link
Member

Choose a reason for hiding this comment

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

Will we ever want keepNames: false?

Copy link
Contributor Author

@jahorton jahorton Jan 11, 2024

Choose a reason for hiding this comment

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

It's not viable on Android 5.0 devices, so yes.

https://caniuse.com/mdn-javascript_builtins_function_name_configurable_true

common/web/lm-worker/build-wrapper.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-wrapper.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-wrapper.js Show resolved Hide resolved
@mcdurdin mcdurdin modified the milestones: A17S29, A17S30 Jan 6, 2024
Copy link
Contributor Author

@jahorton jahorton left a comment

Choose a reason for hiding this comment

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

Prepping additional code-suggestions to apply with the others...

Comment on lines +189 to +190
// https://caniuse.com/mdn-javascript_builtins_function_name_configurable_true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming I'm properly understanding you here, we will not need an issue to track this. From #10257:

export function forES6(config: esbuild.BuildOptions): esbuild.BuildOptions {
return {
...config,
target: "es6",
conditions: ['es6-bundling'],
// Even if they'd be tree-shaken out, esbuild will fall over from their presence
// within an imported `@keymanapp/common-types`.
external: ['timers', 'buffer', 'events'],
// Available with ES6, but not necessarily with ES5.
keepNames: true
};
}

The ES6 target's configuration will re-enable this.

common/web/lm-worker/build-wrapper.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-wrapper.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-wrapper.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-wrapper.js Outdated Show resolved Hide resolved
common/web/lm-worker/build-wrapper.js Outdated Show resolved Hide resolved
@github-actions github-actions bot added web/ and removed web/ labels Jan 8, 2024
@github-actions github-actions bot added web/ and removed web/ labels Jan 8, 2024
@jahorton
Copy link
Contributor Author

jahorton commented Jan 8, 2024

iOS build failure was due to the need to sign a license agreement. No need to hold this up for that.

There was a temporary merge conflict upon merging #10255, but it was auto-resolvable on desktop.

@jahorton jahorton merged commit 1479aa2 into master Jan 8, 2024
3 checks passed
@jahorton jahorton deleted the change/web/lm-worker-bundling-cleanup branch January 8, 2024 08:53
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.239-alpha

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.

3 participants