Experimental DX improvements

[smallsaucepan]

Currently pnpm install installs the required packages from npm (thank you) but then also starts generating esm and commonjs Javascript for the whole project (uhh ... thanks?) This takes 10 - 15 minutes on my laptop.

And it occurs every time you run install. When working on a bug for example, installing or upgrading a dependency to try it out triggers a full rebuild of the whole project. I can cancel it once I see the package is installed, but that doesn't feel right.

Which leads to the question, if Turf is (basically) a TS project, do we need to generate JS at all unless we're preparing a release?

And then thinking more broadly, can we improve the developer experience (DX) of working on Turf? Make it quicker, less wasteful, and more clear what's going on. Spent some time looking at this and below is a summary of the process. Choosing to document it here as while largely (I think) successful, it is a departure from the way we're currently doing things and will require discussion.

Goals are broadly:

1. develop using only TS as much as possible
2. don't generate or build anything until we need to

Stop building JS so often

"prepare": "lerna run build && husky install",

According to NPM scripts prepare runs in a couple of circumstances, including after install. We can split the above into:

"postinstall": "husky install",
"prepublish": "lerna run build",

Running npm install installs husky commit hooks, but doesn't run the build target until we're clearly about to publish a release.

Get unit tests to pass without having to build JS

Right out of the gate NX has the lerna test target depend on build, so remove that dependency:

"test": {
  "dependsOn": [], // used to be "build"
}

With the above done it's possible to run pnpm run test in turf-helpers without building (no JS in the dist/ directory), as helpers depends on no other turf packages.

Running the same in turf-along fails though because along depends on @turf/bearing, etc. This currently works because while the along code is being loaded from turf-along/index.ts all the local dependencies are loaded from generated JS e.g. turf-bearing/dist/esm/index.js. So a lot of our testing is being run against a mixture of TS and JS. Not the worst thing in the world, though I hadn't realised this.

To instead allow turf-along/index.ts to import turf-bearing/index.ts directly, we can add the below to tsconfig.shared.json:

"baseUrl": ".",
"paths": {
  "@turf/*": ["./packages/turf-*"]
}

This wasn't quite working though. The solution seemed to be

{
  "compilerOptions" : {
    "module": "esnext", // used to be "node16"
    "moduleResolution": "bundler" // used to be "node16"
  }
}

By and large package unit tests now work without having to generate JS. This will have an impact on the generated JS that we publish and would need to be investigated for backwards compatibility.

Get type tests to pass

types.ts tests fail though because tsc is still running with the old module and moduleResolution values, and can't see the paths added to tsconfig.shared.json. This is because tsc can't mix tsconfig file options and command line options so they are all configured on the command line. The cleanest solution seemed to be to bite the bullet and add a tsconfig.testTypes.json file to each package,

{
  "extends": "../../tsconfig.shared.json",
  "compilerOptions": {
    "noEmit": true
  },
  "include": ["types.ts"]
}

and then change

"test:types": "tsc --esModuleInterop --module node16 --moduleResolution node16 --noEmit --strict types.ts"

to

"test:types": "tsc -p ./tsconfig.testTypes.json"

Most type tests work now, with a few exceptions.

Reversing some export related workarounds

Remaining type tests that were failing were related to current re-export workarounds in some packages that imported older third party libraries with weird types e.g.

// Get around problems with moduleResolution node16 and some older libraries.
// Manifests as "This expression is not callable ... has no call signatures"
// https://stackoverflow.com/a/74709714

import lib from "sweepline-intersections";

export const sweeplineIntersections = lib as unknown as typeof lib.default;

Removing these (sweepline-intersections and rbush) and simply importing the third party library directly allowed all type tests to pass.

@turf/turf

@turf/turf being an exceptional package - aggregating all the micro-functions and providing a minified bundle - required special treatment.

My thinking here (and happy to hear other points of view) is that for most development work @turf/turf doesn't need to be built. If someone is fixing a bug in @turf/along they can code, test and commit using TS only.

"postinstall": "husky install",
"test": "lerna run test" // runs tests using TS only for everything BUT @turf/turf
"prepublish": "lerna run build && lerna run --scope @turf/turf last-checks",

In other words, only build and run last-checks - which tests the aggregation and minification - when we're doing a release (and probably for CI runs too if we want to be on the safe side).

Summary

A fair read to be sure, though what this covers reflects where Turf probably could be as a developer friendly project. And depending on your opinion, it might also be a more sensible project setup - split steps to be more in line with npm life cycles and don't spend time generating things until we absolutely need to.

Welcome thoughts, criticisms, or questions 😅

[twelch]

I appreciate the step-by-step to help understand what's happening and what you were able to get working. I'm in agreement with this direction.

It sounds like you found an approach that is close to fully working with where things are right now is that right, but still more to figure out? Would the ultimate goal be to finish completely converting to TS? I'm not sure if that was the original intention when the piece-meal introduction of TS started a few years ago before my time, but I gathered from comments that the mixed JS/TS setup that evolved wasn't quite as clean as hoped.

[twelch]

It's really neat that one addition to paths with wildcard works for transforming the import path for all turf modules.

I had been reading about bundler moduleResolution in my own esm conversion. What exactly made you need it? Is it just required to use paths?

[mfedderly]

@rowanwins do you still have thoughts on the JS/TS divide in Turf? I know the last time it came up you were more in favor of preserving the ability to write JS in the Turf repo.

Personally I'd be happy to see consistent TS across the repo.

[mfedderly]

I think we may be able to split change of splitting prepare script into prepublish and postinstall into its own first step before attempting to change the underlying build system. It might be worth changing the scripts and updating the documentation to shorten the pnpm install time and have people remember to run pnpm build in the packages that they change before testing in packages that depend on them. This might get us a reasonable improvement sooner before needing to undertake the larger project proposed here.

I think that this approach might be a little hard to get working while we still have modules being implemented in JS. I'm not entirely sure how lerna handles this, but you can set up turbo (or gradle) to understand that the test script in package x requires the build in package y to have completed. That might allow us to have the minimal set of builds get completed to run the various scripts in a reliable way.

If we did make it to a TS-only repo, I'm pretty confident that project references can help us manage the dependency tree, and monorepolint has support for maintaining these references based on package.json dependencies. I haven't thought about it too hard, but that may allow us to easily get test/type test/bench to work without needing to explicitly build our dependencies.

[smallsaucepan]

Thanks heaps for your comments @twelch and @mfedderly. Answering inline below:

Would the ultimate goal be to finish completely converting to TS?

What I'm suggesting isn't so much to convert completely to TS. It's more to avoid generating JS in dist/ unless we need to. Is there a way to develop - edit, add features, bug fix, and test - without having to generate all the stuff in dist/ with pnpm build?

still more to figure out?

It's actually come together pretty smoothly (surprised Pikachu face). I can fix a bug, add and run tests, and commit - all without having to run build or generate anything in dist/.

Then to prepare a release, the generated code in dist/ looks pretty much identical to the current setup. All the .js, .cjs, .d.ts and .d.cts files are the same (except for the expected changes where importing sweepline-intersections and rbush). The .map files differ so I will double check them.

I had been reading about bundler moduleResolution in my own esm conversion. What exactly made you need it? Is it just required to use paths?

It did seem to be required. From the TS docs it's much the same as what we used to have (node16) except it's more forgiving of relative imports. Which is what I believe the TS runtime falls back to via paths when it sees there is no dist/ directory at the destination. That's the theory at the moment - will confirm with some experimentation.

@rowanwins do you still have thoughts on the JS/TS divide in Turf? I know the last time it came up you were more in favor of preserving the ability to write JS in the Turf repo.

Fairly sure nothing in this approach blocks mixing in JS modules. For example, turf-bbox is TS and happily consumes meta (JS) and helpers (TS) under the new setup. Agree a consistent language across the codebase would be nice. Moving everything to TS first isn't a pre-requisite though.

we may be able to split change of splitting prepare script ... before attempting to change the underlying build system

Happy to take it in a couple of stages, though I don't believe we're really changing the build system. A release is still generated using tsup and the code produced seems to be equivalent.

All we're changing is to not generate a bunch of release related artefacts just to develop and run unit tests.

have people remember to run pnpm build in the packages that they change before testing in packages that depend on them

This is a key point - I don't think people have to run pnpm build. With this new setup, if I edit turf-meta/index.js and then run the tests in turf-bbox/ they pick up my change in meta without having to build anything.

As a next step I'll push my branch so you can all take a look and have a play with it. Clone the repo, npm install, change some TS or JS, and run the tests. See how far you can get without having to run pnpm build!

[smallsaucepan]

Here's the branch: https://github.com/smallsaucepan/turf/tree/experimental-dx

Try:

git clone git@github.com:smallsaucepan/turf.git experimental-dx
cd experimental-dx/
git checkout experimental-dx
pnpm install --frozen-lockfile
cd packages/turf-ellipse/
pnpm run test

This should run the tests without building or generating any JS. Ignore warnings about "Invalid 'main' field in ..." for now.

Add a console.log() call to transformRotate() in turf-transform-rotate/index.js. Immediately run the turf-ellipse tests again, and you should see your new message logged multiple times.

You can also run pnpm run test from the root directly which will run tests for all packages (except @turf/turf). All without generating any unnecessary JS.

That should be enough to get you started. Have a play around and let me know how you go.

[smallsaucepan]

Wondering if anyone has had a chance to have a play with this?

FWIW I've been dog-fooding this approach with recent converting to typescript PRs, and it's worked fine.

[twelch]

I found a little time to play with your branch @smallsaucepan . I can at least confirm it works as instructed. And confirmed running test in turf-bbox (TS module) picks up changes (an introduced bug) in turf-meta (JS module) without a rebuild. And running tests in a TS module picks up changes in other TS modules (expected).

I don't think people have to run pnpm build. With this new setup, if I edit turf-meta/index.js and then run the tests in turf-bbox/ they pick up my change in meta without having to build anything.

This change seems worthwhile. It takes out a level of having to remember to rebuild a module. On top of not having to wait for the entire build up front on install.

Are there any other downsides here?
Silly question, but are there any turf modules that are JS, using a Turf module that is TS? If so, is that covered by this new setup?

[twelch]

I'd say put up a PR

[smallsaucepan]

Thanks @twelch.

are there any turf modules that are JS, using a Turf module that is TS

Yeah several. For example, buffer and interpolate use helpers. Haven't had any problems with those.

Matt mentioned he'd like to have another look at this when he gets a chance in a few weeks. Will wait to hear what that brings and then we can work toward a PR.

[mfedderly]

Ok I've got a few observations already here.

I really want to cache the .nx data across github actions runs, but I think they pretty much don't want you to do that because it sidesteps their cloud offering. I have to think about this some more.

The first is that our nx.json file has "test" dependsOn "build" but that can actually just be "^build" instead because tests don't require the package itself to be built.

The second is that you can execute any task in the repo and specifically include its dependencies with a command like so:

pnpm lerna run test --scope=@turf/along  

If we're willing to use that style of command or wrap it with some syntax sugar, we can probably get away with not modifying the repo very much and we get partial (but correct) builds.

[smallsaucepan]

If we're willing to use that style of command or wrap it with some syntax sugar ...

I don't think that's necessary for what this discussion looking at. We don't need to build anything to run the tests with the changes on the branch above.

Taking this approach also benefits the CI build (perhaps making the nx caching unnecessary) because the action isn't spending 90% of it's time generating two flavours of Javascript for no reason.

Edit: FYI pulled recent updates into the DX branch which triggered a build. Observed the following CI times for comparison:

	master	experimental DX
Install	4m 41s	5s
Test	3m 45s	2m 16s
Overall	8m 37s	2m 30s