-
Notifications
You must be signed in to change notification settings - Fork 211
Description
Overview
We should go through the package.json and tsconfig.json/tsconfig.build.json files and clean up what we can. This issue is a public brain dump of my thoughts, in order to attract others' thoughts 🙂.
package.json
- Remove
build:typesscript - Remove
prelintscript- Remove need to
build:modulebeforetest
- Remove need to
- Fix
docson Windows - Add
srcto"files" - Update
engines.node
tsconfig.json
- Add
"erasableSyntaxOnly": true - Check-up on other settings across
tsconfig&tsconfig.build - Consolidate typescript config into a single
tsconfig.json#493
Details
package.json
scripts
I'm mostly focused on the scripts and pre* automations. Some of it seems rather inefficient.
Lines 81 to 101 in ae88c5e
| "scripts": { | |
| "prebuild": "npm run build:clean", | |
| "build": "npm run build:module && run-p build:browser build:node", | |
| "build:clean": "shx rm -rf dist-node/ dist-browser/ dist-module/", | |
| "build:module": "tsc && rollup -c rollup.worker.config.js", | |
| "build:browser": "rollup -c rollup.config.js", | |
| "build:node": "tsc --project tsconfig.build.json && shx echo \"{\\\"type\\\":\\\"commonjs\\\"}\" > dist-node/package.json", | |
| "build:types": "tsc --outdir dist-module/", | |
| "watch:module": "chokidar \"src/*.js\" -c \"npm run build:module\"", | |
| "watch:browser": "chokidar \"dist-module/*.js\" -c \"npm run build:browser\"", | |
| "predev": "npm run build", | |
| "dev": "run-p watch:module watch:browser dev:serve", | |
| "dev:serve": "serve --listen 8090", | |
| "docs": "rm -rf docs/; typedoc", | |
| "prelint": "npm run build:module", | |
| "lint": "eslint src test .eslintrc.cjs", | |
| "lint:fix": "npm run lint -- --fix", | |
| "prepare": "npm run build", | |
| "pretest": "npm run lint && tsc --noEmit", | |
| "test": "mocha --full-trace test/*.spec.js" | |
| }, |
build: details below.build:types: no longer used,tscnow generates types by default (i.e. inbuild:module).docs: still has the Windows bug detailed in Actually fix windows build #190 (;instead of&).prelint: I don't think we need to builddist-module/in order to lintsrc/andtest/.- At the moment, we do need to build
dist-module/beforenpm run test, since that's whattest/geotiff.spec.jsimports.- I believe this is undesirable.
- At the moment, we do need to build
pretest:tsc --noEmitfeels clunky here; I feel like that part belongs either intestorlint.- I convinced myself that I don't mind
pretest, it's mostly justprelintthat's problematic.
- I convinced myself that I don't mind
- I don't use
watch*ordev*.
build
The build script (and output) feels clunky and confusing. The process after typing npm run build:
"prebuild": "npm run build:clean"- This is fine. I feel like build tools can usually do this now, but "if it ain't broke, don't fix it."
"build": "npm run build:module && run-p build:browser build:node""build:module": "tsc && rollup -c rollup.worker.config.js"- Generate
dist-module/, including.d.tstypes & an inlinedsrc/worker/decode.jsworker
- Generate
"build:browser": "rollup -c rollup.config.js"- Cool, building the UMD bundle... from
dist-module/? Because ofdist-module/worker/create.jsfromrollup?
- Cool, building the UMD bundle... from
"build:node": "tsc --project tsconfig.build.json && shx echo \"{\\\"type\\\":\\\"commonjs\\\"}\" > dist-node/package.json"- Okay... building
dist-node/CJS files fromdist-module/, then creatingdist-node/package.jsonto notate the CJS type. Again fromdist-module/because ofrollup'sdist-module/worker/create.js, I guess? - Importantly, this step disregards
dist-module/*.d.tsfiles and regenerates new ones from the JSDoc comments indist-module/*.jsfiles. This results in a loss of some type information, especially from originally-TypeScript files. However, this doesn't particularly matter, because the"types"are always retrieved fromdist-module/. So we're ultimately generating a lot of files for no reason.
- Okay... building
Writing this out has helped me understand the process a bit, but it still feels like it can be improved. My experience with Vite library mode is only tangentially helpful for geotiff.js—the "help wanted" label is mostly for opinions on building. We can even make a new issue for discussing the build step if preferred.
files
Lines 28 to 32 in ae88c5e
| "files": [ | |
| "dist-module", | |
| "dist-node", | |
| "dist-browser" | |
| ], |
I believe adding src is all that's necessary to enable correct "Go to Definition" jumps, regardless of if the definition is in a .ts or .js file. See #487 (comment) for an example.
engines
Lines 33 to 35 in ae88c5e
| "engines": { | |
| "node": ">=10.19" | |
| }, |
Do we still support NodeJS >= 10.19? Do we want to?
I bumped to >= 14.0.0 in #494.
Now I'm leaning towards >= 22.18 for TypeScript + erasableSyntaxOnly, but we don't need to limit it that much yet (especially while still building dist-node/).
exports
Lines 20 to 26 in ae88c5e
| "exports": { | |
| ".": { | |
| "import": "./dist-module/geotiff.js", | |
| "require": "./dist-node/geotiff.js", | |
| "browser": "./dist-browser/geotiff.js" | |
| } | |
| }, |
We might be able to do something like point "node" "exports" at src/—although only for NodeJS >= 22.18 (see below), and I'm not sure if conditional exports based on the version are possible.
tsconfig.json
Related to #493.
Lines 2 to 23 in 780b82e
| "compilerOptions": { | |
| "outDir": "dist-module", | |
| "target": "ES2020", | |
| "lib": ["ES2020", "DOM", "ES2021.WeakRef"], | |
| "module": "ES2020", | |
| "allowJs": true, | |
| "checkJs": true, | |
| "declaration": true, | |
| "declarationMap": true, | |
| "sourceMap": true, | |
| "importHelpers": false, | |
| "noImplicitAny": true, | |
| "noUnusedLocals": true, | |
| "noUnusedParameters": true, | |
| "strict": true, | |
| "strictNullChecks": true, | |
| "skipLibCheck": true, | |
| "moduleResolution": "bundler", | |
| "esModuleInterop": false, | |
| "inlineSources": false, | |
| "downlevelIteration": true, | |
| }, |
erasableSyntaxOnly
Based on the discussions in #487, I strongly feel that we should enable erasableSyntaxOnly to preserve native execution in NodeJS (>=22.18)
Others
We'd need to go through the other options to figure out what we actually need. For example, "strict": true is already enabling a few of the other type checking options (noImplicitAny, strictNullChecks). Maybe the redundancy is desirable since it's more explicit, but I'd rather trim out as much as we can.
A non-exhaustive list of potential changes:
target&lib: are we happy with"ES2020"and["ES2020", "DOM", "ES2021.WeakRef"], and is it the minimum we can/want to go?esModuleInterop: I'm mostly just curious why this isfalsefortsconfig.json, thentruefortsconfig.build.json. Does it need to be that way, or what benefit is that providing?downlevelIteration: I'm not sure if we need this but it's probably not hurting.
tsconfig.build.json
geotiff.js/tsconfig.build.json
Lines 2 to 19 in 780b82e
| "compilerOptions": { | |
| "target": "ES2020", | |
| "module": "commonjs", | |
| "outDir": "./dist-node", | |
| "lib": ["ES2020"], | |
| "allowJs": true, | |
| "declaration": true, | |
| "declarationMap": true, | |
| "sourceMap": true, | |
| "importHelpers": false, | |
| "skipLibCheck": true, | |
| "strict": false, | |
| "strictNullChecks": true, | |
| "moduleResolution": "node", | |
| "esModuleInterop": true, | |
| "inlineSources": false, | |
| "downlevelIteration": true | |
| }, |
Provided for comparison.
The only real differences are lib, module, moduleResolution, esModuleInterop, and strict. We could probably get by with just tsconfig.json and CLI options for build:node.