Introduced new developer experience efficiencies #2702

smallsaucepan · 2024-08-16T06:02:44Z

Reasonably major attempt to streamline the Turfjs development environment for contributors. Overall concept is to avoid generating Javascript build artefacts unless we absolutely have to.

Summary of changes. Some may be contentious, so happy to debate or justify in more depth:

Uses paths config (tsconfig.shared.json) to help build and test tools / editors resolve TS dependencies between packages more directly. Used to have to build dependent packages to JS first and load those
Removed need to build project before testing. Major time saver for Turf org developers
Adopted newer npm lifecycle targets to avoid building things before we need them. Doesn't automatically generate two versions of JS on npm install for example
Added static type checking for all projects. TS packages type check index.ts directly, JS packages test a types.ts scaffold file as the entry point. Detects errors introduced in locally edited dependent packages
Removed most existing types.ts files. Approach above hits the index.ts file directly, and most types.ts file weren't doing much more than serving as an entry point for tsc
Rejigged rollup + last-tests targets in @turf/turf as cdnBundle. Felt this more accurately describes what that's doing
ESM and CJS Javascript, and minified CDN bundle, aren't generated until we are actually preparing to publish a release
Updated module and moduleResolution to esnext and bundler respectively (tsconfig.shared.json). This didn't seem to affect the generated JS output, as the target remains at es2017
At end of CI run all checked in code will have been unit tested and type checked. Artefacts generated for a release (EJS, CJS, etc) will no longer be tested every CI run. However, those are the products of automatic processes so testing every time would rarely uncover problems not already seen earlier in unit tests.

Further background discussion #2629 and some relevant comments on now superceded PR #2682.

project infrastructure
Have read and followed the steps for preparing a pull request.

…ckage, rather than specifying the arguments on the command line.

…facts. See discussion Turfjs#2629

…o building to work. Simplifies things overall.

…lined in dist/esm/index.js files, greatly increasing their size.

…re causing problems in a previous build because skipNodeModulesBundle was true. That's now been removed and @turf/turf does build fine without them.

…ome degree (except @turf/turf). TS projects we statically type check the index.ts itself. JS projects we type check a minimal types.ts file against the index.d.ts definition.

…ipt infers type dependencies of imported types. These must have surfaced due to this PRs new approach to linking package dependencies. Solution was to explicitly define a few return types that previously were being inferred. Should be fixed permanently in typescript 5.5, though nothing really for us to revert. See microsoft/TypeScript#47663 (comment) Also synced some JSDoc with the function's actual type definition.

…ve. Adding empty marchingsquares d.ts to isolines (same as we've done for isobands) to retire a ts-expect-error. Also believe prepublishOnly better target to use than prepublish.

mfedderly · 2024-08-19T18:29:52Z

.monorepolint.config.mjs

@@ -50,6 +46,56 @@ const JS_TAPE_PACKAGES = TAPE_PACKAGES.filter(

 export default {
  rules: [
+    fileContents({


https://monorepolint.com/docs/rules/standard-tsconfig/ maybe we should use standardTsconfig now instead of fileContents. My biggest issue with the current monorepolint is that we've got hand-managed lists of files. Although most of our packages only use a single file, its definitely unexpected if I add another file and the import doesn't work until it gets added in here (and further extends the proliferation of special cases in this file.

I wonder if we can get away with something like this and be a little more forgiving of adding extra files.

{ "include": ["index.js", "index.ts", "lib/*", "*.d.ts"], "exclude": ["index.d.ts"] }

Will check this idea out. Thanks 👍

mfedderly · 2024-08-19T18:33:46Z

packages/turf-boolean-touches/types.ts

-  [0, 0],
-  [10, 10],
-]);
-booleanTouches(pt, line);


I think the point of these was to help guard against API breaks in the types? Kind of like what DefinitelyTyped does. I'm not sure that that's a super compelling argument here with a relatively basic amount of syntax expressed in this file. The last-checks step is grabbing the code in the examples and checking that the example compiles. Thoughts? (I do kind of want each package to test its own example, documentation generation, etc).

Do you mean like a form of API locking between major versions? I thought so too at first. However, I could easily narrow the signature of booleanTouches to Feature, Feature and this test would still pass. As is, I don't think they really protect us much.

I do kind of want each package to test its own example, documentation generation, etc).

Definitely. That is still the case. The tsc --noEmit in each package is checking to see if the module compiles. We don't need to actually call our method to have tsc examine it though. Just importing seems to be enough.

We can add a bunch of example invocations if we want. All we're doing though is proving we know how to call our own library. It's not testing the type safety of the implementation any better.

mfedderly · 2024-08-19T18:35:48Z

packages/turf-clusters-dbscan/index.ts

@@ -2,7 +2,7 @@ import { GeoJsonProperties, FeatureCollection, Point } from "geojson";
 import { clone } from "@turf/clone";
 import { distance } from "@turf/distance";
 import { degreesToRadians, lengthToDegrees, Units } from "@turf/helpers";
-import { rbush as RBush } from "./lib/rbush-export.js";
+import rbush from "rbush";


Its nice that we can get rid of the import hacks, I'm still trying to fix the underlying typings still.

mfedderly · 2024-08-19T18:36:54Z

packages/turf-clusters-dbscan/types.ts

-// Custom Properties
-clustered.features[0].properties.cluster;
-clustered.features[0].properties.dbscan;
-clustered.features[0].properties.foo;


This is an example of a type test that might be more likely to catch typings breaks than just the example for the package.

This type checks fine as is. I could make that features[999] though and tsc would still say it's correctly typed. That would error at runtime, so maybe it belongs more in test.ts?

A test worth adding might be to uncomment the line dbscan = 'foo' //= ... and add a ts-expect-error comment, which (I'm pretty sure) would generate a compile time error if that line ever started working.

At least that way we could tell if the property name constraint was ever weakened.

mfedderly · 2024-08-19T18:40:15Z

packages/turf-geojson-rbush/tsconfig.testTypes.json

+    "noEmit": true
+  },
+  "files": ["types.ts"]
+}


If you wanted to consolidate this further, you can make a root tsconfig.testTypes.json which has all of this and then each package can just be an extends directive. With ${configDir} things like files are now possible to define once. https://devblogs.microsoft.com/typescript/announcing-typescript-5-5/#the-configdir-template-variable-for-configuration-files

That would be great! Wasn't able to find a way to avoid these until now. I will refactor.

#2684 is the Typescript upgrade for Turf

Shall we get #2684 merged first then, I'll bring that into this PR, and then rejig to use the configDir stuff?

Yeah feel free to merge it whenever you want, I hit the update button. Actually though this brings up a bit of an interesting point, a new typescript version (or relevant package.json change?) should force everything to rebuild/test.

If you mean in CI, then yes it did.

mfedderly · 2024-08-19T18:42:04Z

packages/turf-geojson-rbush/tsconfig.testTypes.json

+  "compilerOptions": {
+    "noEmit": true
+  },
+  "files": ["types.ts"]


I'm actually a little surprised this doesn't need to declare index.js, I must be missing something about how the paths directive works or something.

I don't think tsc can type check the JS implementation at all. So for the JS projects I believe it uses types.ts as an entry point, loads up index.d.ts (infers that's what you really mean when asking to import from index.js), and then checks what it can.

Right but shouldn't index.d.ts have to be listed in files in that case?

files seems more to be the top level entry points where you want tsc to start compiling. From there it uses whatever the module resolution policy is to find all the dependencies.

https://www.typescriptlang.org/tsconfig/#files I guess it doesn't say that it has to be exhaustive, but I always read that as needing to list every file. 🤷

mfedderly · 2024-08-19T18:44:58Z

packages/turf-line-slice-along/types.ts

@@ -0,0 +1 @@
+import { lineSliceAlong } from "./index";


Probably needs some usage if this file exists.

Unless I'm missing your point, this is probably as good as we can make it, at least for a JS project. We could add example calls to lineSliceAlong, but all that's will confirm is we've written the example calls to match the signatures defined in index.d.ts

Alternative tack: do we not bother doing static type checking in our remaining JS packages? I don't think it's really adding much.

Yeah if we aren't getting benefit from types.ts testing, maybe we can just remove it entirely. Something like https://api-extractor.com/ might be better for making sure we don't actually break APIs than these manual tests.

Ok so I dug in on one of the types.ts files and it originally came from here which is part of the early Typescript implementation, so maybe in 2024 they are less relevant.

74140e8

So, sounds like we're happy to ditch the types.ts for remaining JS projects, knowing that we'll have actually useful static type checking once they're converted to TS?

API extractor worth investigating too 👍

I'd like to get more consensus around this decision, especially since I wasn't around when the decision was made to include them. I don't want to delay this PR indefinitely though.

Sure thing. I'll proceed as if we will retire them for the remaining JS projects, and if justification comes up to keep them, it's easy to revert before we merge this PR.

The above is done. To further simplify configuration (MRL mainly) all packages now run tsc. For the JS projects though it's pretty much a no-op as it can't make any guarantees about the type safety of the JS code.

mfedderly · 2024-08-19T18:45:02Z

packages/turf-line-slice/types.ts

@@ -0,0 +1 @@
+import { lineSlice } from "./index";


Probably needs some usage if this file exists.

mfedderly · 2024-08-19T18:49:48Z

packages/turf/package.json

-    "test": "echo '@turf/turf tests run in the last-checks step'"
+    "build": "tsup --config ../../tsup.config.ts",
+    "cdnBundle": "npm-run-all cdnBundle:minify cdnBundle:checks",
+    "cdnBundle:checks": "tsx test.ts && tsx test.example.js && tsx test.ts",


Double tsx test.ts here ^

Just being extra cautious. Will fix this.

smallsaucepan · 2024-08-20T14:42:19Z

Thanks for that feedback @mfedderly. Let me know what you think about the type test comments. Everything else I'll refactor in coming days and push up.

…packages the same. Only exception now is @turf/turf. Run tsc static type checking in all packages (except @turf/turf), even the JS ones. It's a no-op for JS, though standardises our package structure somewhat. Added a bunch of standard devDependencies to all projects including typescript, tslib, bench, tape, npm-run-all. Same rationale as above. Upgraded to typescript 5.5.4. Retired types.ts tests as most of them weren't adding much value any more now that we use tsc --noEmit to type check TS code.

…inting of generated code (escheck). We now run the former during top level test, and the latter only during pre-publish after we've run a top level build. Upgraded the eslint plugin to match new typescript version, fixed a couple of minor "new" lint errors.

smallsaucepan added 11 commits June 24, 2024 00:09

Configuring types.ts tests using a dedicated tsconfig file in each pa…

a924d5e

…ckage, rather than specifying the arguments on the command line.

Experimenting with changes to when and why we build different JS arte…

180efa7

…facts. See discussion Turfjs#2629

Need to remove some old weird type workarounds for the new approach t…

0821730

…o building to work. Simplifies things overall.

Enabling skipNodeModulesBundle was causing dependent modules to be in…

232efc7

…lined in dist/esm/index.js files, greatly increasing their size.

These packages weren't actually needed by @turf/turf. Suspect they we…

d966fd8

…re causing problems in a previous build because skipNodeModulesBundle was true. That's now been removed and @turf/turf does build fine without them.

Merge branch 'master' into experimental-dx

5303d1f

Merge branch 'master' into experimental-dx

19780d0

Merge branch 'master' into experimental-dx

18710a6

Type testing using tsc updated / added for all packages to at least s…

70a8580

…ome degree (except @turf/turf). TS projects we statically type check the index.ts itself. JS projects we type check a minimal types.ts file against the index.d.ts definition.

Refactoring "last-checks" into "bundleCdn" which seems more descripti…

cd4b7d6

…ve. Adding empty marchingsquares d.ts to isolines (same as we've done for isobands) to retire a ts-expect-error. Also believe prepublishOnly better target to use than prepublish.

smallsaucepan requested review from mfedderly and twelch August 16, 2024 06:15

smallsaucepan mentioned this pull request Aug 16, 2024

Inline last-checks into @turf/turf's test script #2690

Open

mfedderly reviewed Aug 19, 2024

View reviewed changes

smallsaucepan mentioned this pull request Aug 22, 2024

Upgrade typescript #2684

Merged

smallsaucepan added 3 commits August 22, 2024 14:25

Merge branch 'master' into experimental-dx

2fde264

smallsaucepan marked this pull request as draft September 28, 2024 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced new developer experience efficiencies #2702

Introduced new developer experience efficiencies #2702

smallsaucepan commented Aug 16, 2024

mfedderly Aug 19, 2024

mfedderly Aug 19, 2024

smallsaucepan Aug 20, 2024

mfedderly Aug 19, 2024

smallsaucepan Aug 20, 2024

mfedderly Aug 19, 2024

mfedderly Aug 19, 2024

smallsaucepan Aug 20, 2024

mfedderly Aug 19, 2024

smallsaucepan Aug 20, 2024

mfedderly Aug 20, 2024

smallsaucepan Aug 21, 2024

mfedderly Aug 22, 2024

smallsaucepan Aug 22, 2024

mfedderly Aug 19, 2024

smallsaucepan Aug 20, 2024

mfedderly Aug 20, 2024

smallsaucepan Aug 21, 2024

mfedderly Aug 22, 2024

mfedderly Aug 19, 2024

smallsaucepan Aug 20, 2024

mfedderly Aug 20, 2024

mfedderly Aug 20, 2024

smallsaucepan Aug 21, 2024

mfedderly Aug 22, 2024

smallsaucepan Aug 22, 2024

smallsaucepan Aug 23, 2024

mfedderly Aug 19, 2024

mfedderly Aug 19, 2024

smallsaucepan Aug 20, 2024

smallsaucepan Aug 23, 2024

smallsaucepan commented Aug 20, 2024

Introduced new developer experience efficiencies #2702

Are you sure you want to change the base?

Introduced new developer experience efficiencies #2702

Conversation

smallsaucepan commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallsaucepan commented Aug 20, 2024