Build process rewrite

[brianmhunt]

Background

Flowing from #130 and started implementing in #111, I am revisiting the build process. The last time we looked at this the build process was very challenging for a few reasons:

1. it required a tool with potentially lots of plugins (e.g. rollup, babel, Webpack), some of which were rather fickle
2. the build targets were not clearly defined/definable in the Javascript ecosystem

Since that last go-round, the ecosystem has changed and stabilized. ESM is now a relatively sure-thing in modern browsers and all recent build tools, and there are alternative build tools. As well, how npm, node, and build tools export packages has increased and become more stable/standardized.

We also have Deno in the landscape, to think about.

Objectives

• O1: Simplify the build process
  • R1: Simpler build tool
  • R2: Separate packages/ from builds/
  • R3: Test the build targets
• O2: Clarify the build targets
  • R1: Typescript (for internal and external building)
  • R2: CommonJS (ES6)
  • R3: ESM (ES6)
  • R4: (builds only) browser.min (ES6)

Technical Design

I've landed on replacing rollup with esbuild because it simplifies the build process substantially.

I tried replacing learn with Makefiles but the interdependencies were too complex, and learn works quite well for task delegation.

The test process requires switching to use a karma-esbuild, which seems to work quite well. There's some config overlap in what's passed to esbuild on the command line for building and what's in the karma.conf.

The build process is somewhat fickle, so I'm adding tests to build/reference/spec that verify that the various processes export/import TKO as anticipated.

Alternatives considered

• Continue using rollup, but the simplicity and speed of esbuild is compelling, and we are looking to using esbuild in a browser.

[brianmhunt]

Note re build topology

Lerna

I tried using lerna exec to build all the items in order. Unfortunately that seems to fail sporadically, seemingly because learn considers devDependencies and dependencies to be equivalent.

npm

I looked at npm exec but it doesn't seem to work for workspaces / topological ordering.

yarn

I tried switching to yarn and using the yarn workspaces-tools foreach but I only just realized that requires yarn v2 (i.e. "berry").

Trying yarn v2 just seemed to error on install:

➤ YN0000: └ Completed in 23s 999ms
➤ YN0000: Failed with errors in 26s 543ms

Had it worked, then something like yarn workspaces foreach -pit run make but alas, this fails with Invalid subcommand. Try "info, run". I also noticed it installed a number of config files .yarnrc, .yarn/..., where .yarn was 1.6MB and almost certainly not necessary to include in the repo (though the purpose of these wasn't clear).

rush

I'd never heard of this, but it has a number of smells considering the simple use-case we have, including the complex config. It seems to be overkill here.

pnpm

This seems to lack the basic ability to call an arbitrary command in any given directory.

Workspace-tools

I wonder if it's possible to directly use https://github.com/kenotron/workspace-tools

hand-rolling

All of the above leads me to believe that we quite possibly want to go back to hand-rolling the dependency chain detection. Which is somewhat madness, if you ask me.

[brianmhunt]

Makefile meta

Having tried all the above, I'm going to look at building a .mk file that'll be used to build dependencies in-order.

It'll go something like this:

1. Makefile will create tools/deps.mk if it doesn't exist by calling tools/create-deps.js
2. The create-deps.js will parses every package/* and add it to a dependency chain
3. build.mk will include tools/deps.mk
4. in addition to the dependency chain, tools/deps.mk will have a all:: target that compiles every package

The dependencies will be targets that look something like this:

packages/observable: packages/observable/dist/index.js packages/utils

packages/observable/dist/index.js:
     cd packages/observable; $(MAKE)

packages/utils: packages/utils/dist/index.js

packages/utils/dist/index.js:
    cd packages/observable; $(MAKE)

We can generate the dependency list by parsing package.json for dependencies.

Build Dependency Chain

In build.mk we have dist/index.js as a target. It may be require a number of other packages to be built first.

Example:
In the package @tko/observable, dist/index.js ought to depend on @tko/utils. The way to express this dependency in Makefiles is by adding a target e.g.

@tko/observable: @tko/utils

However those dependencies won't know how or when to build, so we have to give file-system based dependencies e.g. on their index.js file with a command to build:

@tko/observable: @tko/utils ../observable/dist/index.js

../observable/dist/index.js:
   cd ../observable; $(MAKE)

The problem here is that this is a circular loop: in the packages/observable directory:

1. dist/index.js depends on @tko/observable to build up the dependency chain
2. @tko/observable depends on ../observable/dist/index.js
3. ../observable/dist/index.js triggers cd ../observable; make, which is step 1.

The issue is that including build.mk for a package should have dist/index.js depend on other packages, but not itself. It's not immediately obvious how to express this in Make.

[brianmhunt]

I tried a DIY like this, but I think the solution might be to change devDependencies to peerDependencies (untested):

#!/usr/bin/env node
//
// Generate deps.mk, so we've a proper
// topological sort of the dependencies.
//
// See: https://github.com/knockout/tko/issues/134
//
// This will eventully be replaceable by something like:
//
//   yarn workspaces foreach
//

const path = require('path')
const fs = require('fs')

const packagesPath = path.join(__dirname, '..', 'packages')

const buf = [`# Auto-generated by create-deps
# Last generated: ${new Date().toISOString()}
`]

const include = pkgPath => {
  const modulesPath = path.join(__dirname, '..', pkgPath)
  const modules = fs.readdirSync(modulesPath)
  const parse = module => JSON.parse(fs.readFileSync(
    path.join(modulesPath, module, 'package.json')))

  for (const pkg of modules) {
    const package = parse(pkg)
    const deps = Object.keys(package.dependencies || [])
      .filter(d => d.includes('@tko'))
      .map(d => `${d.replace(`@tko`, packagesPath)}/dist/index.js`)
      .join(' ')

    buf.push(`
// ${pkgPath}/${pkg} => ${package.name}
${package.name}:

../${pkg}/dist/index.js: ${package.name}/commonjs
\tcd ../${pkg}; $(MAKE)

${package.name}/commonjs: ${deps}
`)
  }
}

include('packages')
include('builds')


console.log(buf.join(''))

But this is rather fickle and the next dev to come along (quite possibly me) will be head-scratching for a while with this.

[brianmhunt]

Lerna with peerDependencies

Unfortunately while it builds fine locally, building appears to still be broken in CI, starting with this error:

@tko/observable: > src/observable.js:7:7: error: Could not resolve "@tko/utils" (mark it as external to exclude it from the bundle)

[brianmhunt]

✅ Unlinked packages

It looks like the issue was with the @tko packages not being linked in Github's build directory.

1. change devDependencies to peerDependencies
2. change to npm 7 (so we have npm workspaces)
3. run npm i to link the peer dependencies before running build, so esbuild will resolve them in node_modules/

[brianmhunt]

Testing

Now that I'm through the build process, I'm trying testing.

The CircleCI + SauceLabs config was antiquated and seemed to time out. I looked at a few options:

• Trigger: CircleCI, Github Actions
• Browser runner: SauceLabs, LambdaTest, Browserstack
• Test Runner: Karma, Cypress

Since we've Mocha and Jasmine (an ancient version) tests we're somewhat hamstrung. Eventually I've considered moving everything to Jest with e.g. jest-codemods, but that's a separate effort.

CircleCI

CircleCI seems to work ok, but it's an extra external dependency, so I wanted to try Github Actions to remove that support overhead. If more users start working on this, we can manage their respective access with Github users.

Github Actions / SauceLabs

The Github Actions seem to work fine, though it took an effort to get them up-and-running with SauceLabs. We still get a timeout disconnect. I've temporarily disabled SauceLabs to try and run things locally in e.g. Electron, figuring that would be easier to set up.

Github Actions / Electron

After some effort figuring out how to get headless Electron working, it seems that there's an error that occurs for no apparent reason in the headless setup but that does not occur when run locally, e.g.:

@tko/binding.foreach: Electron 12.0.9 (Linux x86_64) focus does not preserves primitive targets when re-ordering FAILED
@tko/binding.foreach: 	Uncaught AssertionError: expected <input> to equal <body>
@tko/binding.foreach: 	  <!-- The scripts need to be in the body DOM element, as some test running frameworks need the body
@tko/binding.foreach: 	       to have already been created so they can insert their magic into it. For example, if loaded
@tko/binding.foreach: 	       before body, Angular Scenario test framework fails to find the body and crashes and burns in
@tko/binding.foreach: 	       an epic manner. -->
@tko/binding.foreach: 	  <script src="context.js"></script>
@tko/binding.foreach: 	  <script type="text/javascript">
@tko/binding.foreach: 	    // Configure our Karma and set up bindings
@tko/binding.foreach: 	    window.__karma__.config = {"args":[],"useIframe":true,"runInParent":false,"captureConsole":true,"clearContext":true,"mocha":{},"originalArgs":[]};
@tko/binding.foreach: 	
@tko/binding.foreach: 	    window.__karma__.setupContext(window);
@tko/binding.foreach: 	

If I were to speculate I'd say that the test was depending on some timing that differs when run in headless Electron e.g. perhaps requestAnimation.

[brianmhunt]

Given the difficulty I'm having with CI I'm going to just turn it off right now and re-evaluate the thing with a bit more dignity i.e. where it's not blocking a rewrite of the build process.

[brianmhunt]

The Electron issue may be this: electron/electron#9567.

[phillipc]

I think it is done with #204 and #209.
In the future, we could switch to Lerna to Nx.