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

chore(web): replace lerna with npm workspaces and ts projects 🎡 #6525

Merged
merged 105 commits into from
Jun 13, 2022

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Apr 12, 2022

Fixes #6320.

Part 1 of moving from lerna to a simpler, maintained monorepo solution, using TypeScript Projects and NPM Workspaces.

There is more work to be done here. At this point, KeymanWeb builds unminified and runs without errors (locally at least!), but the built file is substantially different, mostly in include order.

Using TypeScript Projects, we move away from the need to run build scripts in various locations for almost all the Typescript modules. npm install or npm ci is needed just once at the top level of the repo. The TypeScript project references mirror the npm references. A discussion may be worthwhile on how we link the two (or if we do) -- this seems to be an open question for TypeScript currently.

On my machine, this reorg reduces the KeymanWeb build from several minutes to about 15 seconds! There may be more to do though.

TODO

For the build script and other cleanup phases (not part of this PR chain):

Note, remaining TODO items moved to #6268 (may be moved again from there).

Whew, that's most of the stuff I noticed!

Fixes #6320.

Part 1 of moving from lerna to a simpler, maintained monorepo solution,
using TypeScript Projects and NPM Workspaces.

There is more work to be done here. At this point, KeymanWeb builds and
runs without errors, but the built file is substantially different,
mostly in include order.

Using TypeScript Projects, we move away from the need to run build
scripts in various locations for almost all the Typescript modules.

TODO: Embedded versions and tests have not been verified.

TODO: developer/server is not yet verified.

TODO: developer/js (needs a rename!) is not yet verified.

TODO: Currently, the predictive-text folder needs refactoring to move
the construction of the worker wrapper out of the Predictive Text build
and into the final assembly of keymanweb.js (as it should be valid to
run it as a separate .js anyway).

TODO: Most of the `<reference>` paths need to be re-verified. Ideally
there should be no references outside the current module for any given
.ts.

TODO: The embedded vs browser vs node (headless) builds should be tidied
up for consistency so that it's obvious what depends on what. This is
currently messiest in the predictive-text folder, where the output names
diverge from the filenames and the various files are mixed in the same
folder (as evidenced by the exclusions listed in each tsconfig.json).

TODO: `npm install` should be removed from most build scripts and
instead `npm ci` (#6196) should be run only once from the top-level
folder for any given build. I've had eliminated side-effects from the
`install` action for npm, which makes it easier to reason about state.

TODO: verify_npm_setup and related functions can probably be eliminated.

TODO: most of the build scripts should be largely eliminated for web.

TODO: several ts projects use inconsistent output folders.

TODO: it may be possible to generate a .d.ts for models/types so that
we can use a consistent reference for those as well.

TODO: build.sh, tsconfig.json should always be in the module's top-level
folder, not in a subfolder such as src (e.g. see input-processor/src,
keyboard-processor/src, web/source).

TODO: resources/web-environment should be in common/web.

TODO: other js node_modules imports should be wrapped like es6-shim.

TODO: fix up the publish code for npm modules

TODO: eliminate version numbers from package.json if possible?

Whew, that's most of the stuff I noticed!
@mcdurdin mcdurdin added this to the B15S6 milestone Apr 12, 2022
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Apr 12, 2022
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Apr 12, 2022

User Test Results

Test specification and instructions

@mcdurdin mcdurdin changed the base branch from master to beta April 12, 2022 11:18
@jahorton
Copy link
Contributor

  • npm install should be removed from most build scripts and instead npm ci [...]

Shouldn't this be a command-line option on our build scripts instead? Or would we need to manually go out of our way to manually run npm install every once in a while when updates are needed?

Keep in mind, that's a not-insignificant amount of data each npm ci run, which could matter a bit whenever stuck on a metered connection, for example.

Maybe we even have the ci version as default, with install the option... but I'm hesistant to drop install from the scripts entirely.

@jahorton
Copy link
Contributor

  • most of the build scripts should be largely eliminated for web.

Are you suggesting that we'll be able to have the isolated component tests run from the main project's folder? I'd think we'd need to address the original idea of #5024 in order to do so successfully.

Each package's folder has very useful unit tests, and I'd think we'd need their build scripts in order to keep 'em running at this time.

@jahorton
Copy link
Contributor

jahorton commented Apr 12, 2022

When I cloned a new copy of the repo to examine this - so that it doesn't pollute my main working area - I got this error during the Web build:

10:06:42 AM - Failed to parse file '../resources/web-environment/tsconfig.json': C:/keymanapp/keyman-reorg/resources/web-environment/environment.inc.ts does not exist.

Obviously, not the hardest thing to resolve. But something of note.

Next issue:

/c/keymanapp/keyman-reorg/web/source
cat: ../../common/predictive-text/build/intermediate/index.js: No such file or directory

It sounds as if the WebWorker-internal workspace is not being built. (It has its own ts-config.json.) Kinda need that for predictive text to work properly... and this means that Web isn't being fully compiled as of yet.

My thoughts toward a possible resolution:

  • We may need something similar to your es6-shim wrapper for the WebWorker internals.
  • To facilitate that...
    • common/predictive-text/worker should become its own, standalone package - perhaps lm-worker-internals?
      • Publishes the folder's build products - the code that gets "wrapped" in the outer build.sh.
    • New package: wrapped-worker:
      • A local-only NPM package that 'builds' by running the build-wrapped-worker function from common/predictive-text/build.sh, distributing its results
      • Obviously, depends on the published products from lm-worker-internals.
      • Plays a similar role to your es6-shim wrapper.
    • From there, lexical-model-layer should be able to path-link to the wrapped-worker TS project.

It's quite possible that this could be simplified; I just based this on my current level of understanding of the new "toys" you're "playing" with. I'm fairly certain this breakdown would work if what you're already doing works for es6-shim.


Both issues above became evident rapidly because it's a clean workspace (fresh checkout).

@mcdurdin
Copy link
Member Author

Thanks for the quick review @jahorton. Some quick notes:

  • The way I understand it, npm ci is broadly the same as npm install in terms of bandwidth requirements. The difference is that npm install can update package versions in package-lock.json, whereas npm ci only installs the exact version listed in the package-lock.json. Changing package versions, even minor and patch updates, should never be something done at time of build but rather something that happens as a part of our maintenance and as an active choice.

  • Yes, there are at least two dependencies that cannot be automatically built at this time with tsc -b (lm-worker-internal (or whatever we end up calling it), and web-environment). I think the best way forward for web-environment is to package the build steps for them in a postinstall script (though I hate the side-effect). npm ci at the root will then trigger the builds for it.

    But for lm-worker-internal, I am not sure that will do the job properly. Any change to its code would need a rebuild but that would not happen automatically with tsc -b in the web build, as it is manually appended to keymanweb.js (due to its wrapping). That's kinda annoying, so I am inclined to manually trigger its build from web's build.sh (it can still use tsc -b so the time cost should be low).

    We can change strategy later if it makes sense to do so!

This starts the reorg required for splitting the lm worker into its own
module. Tests are still not working. But builds are going.

Some decisions to be made around the final path for wrapping of the
code. I think it may be possible to do something a whole lot more
elegant by moving the wrapping process into the KMW build and leaving
the LM Worker output as a pure tsc output -- given only KMW wants the
wrapped worker anyway (tests aside). The actual wrapping code could
still be in the lm-worker folder.
@mcdurdin mcdurdin modified the milestones: B15S6, B15S7 Apr 15, 2022
@mcdurdin mcdurdin changed the title chore(web): replace lerna with npm workspaces and ts projects chore(web): replace lerna with npm workspaces and ts projects 🎡 Apr 15, 2022
Starts to resolve the chaos of putting version numbers into every
package.json file.

Adds a new keyman-version package which mimics the
version information in build-utils.sh. This will eventually replace
resources/web-environment.

Fixes the build for the lexical model compiler tools to remove refs to
package.json version and use keyman-version instead.

More fixes coming in a follow-up.
jahorton and others added 7 commits May 30, 2022 08:29
chore(web): restores previous test-setup patterns for Web unit tests 🎡
…nvironment

chore(web): replaces web-environment with keyman-version 🎡
…ndling

chore(web): lm-worker "test stub" code elimination 🎡
…developer

chore(web): adapts keyman-version bundling in Developer products 🎡
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels May 30, 2022
@jahorton jahorton dismissed their stale review May 31, 2022 02:54

Changes have been addressed. Will convert to "approve" once user tests are positive.

@mcdurdin
Copy link
Member Author

Changes have been addressed. Will convert to "approve" once user tests are positive.

Workflow-wise, we don't merge until all three of the following are satisfied:

  • passing build
  • user tests passed
  • code reviews approved.

So please go ahead and approve if you are satisfied with the code changes; there is no need to wait for user tests to pass. (Note: if changes are required as a result of user testing, then we can dismiss reviews as required.)

@mcdurdin
Copy link
Member Author

mcdurdin commented Jun 3, 2022

Test Results

Copy link
Contributor

@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.

While I have seen a couple of interesting issues pop up from some of the app tests... absolutely none of it has been related to Web code, let alone changes from all the 🎡 work.

I'm comfortable giving this the LGTM at this point.

@mcdurdin mcdurdin modified the milestones: A16S3, A16S4 Jun 11, 2022
@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Jun 13, 2022
@mcdurdin mcdurdin merged commit 47da768 into master Jun 13, 2022
@mcdurdin mcdurdin deleted the chore/web/no-more-lerna branch June 13, 2022 03:39
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 16.0.12-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.

chore(common): move from lerna to alternative monorepo solution
3 participants