Build: Output package type declarations #18942
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| const _ = require( 'lodash' ); | ||
| const path = require( 'path' ); | ||
| const fs = require( 'fs' ); | ||
| const execa = require( 'execa' ); | ||
|
|
||
| /* eslint-disable no-console */ | ||
|
|
||
| const repoRoot = path.join( __dirname, '..', '..' ); | ||
| const tscPath = path.join( repoRoot, 'node_modules', '.bin', 'tsc' ); | ||
|
|
||
| // lint-staged passes full paths to staged changes | ||
| const changedFiles = process.argv.slice( 2 ); | ||
|
|
||
| // Transform changed files to package directories containing tsconfig.json | ||
| const changedPackages = _.uniq( | ||
| changedFiles.map( ( fullPath ) => { | ||
| const relativePath = path.relative( repoRoot, fullPath ); | ||
| return path.join( ...relativePath.split( path.sep ).slice( 0, 2 ) ); | ||
| } ) | ||
| ).filter( ( packageRoot ) => | ||
| fs.existsSync( path.join( packageRoot, 'tsconfig.json' ) ) | ||
| ); | ||
|
|
||
| try { | ||
| execa.sync( tscPath, [ '--build', ...changedPackages ] ); | ||
| } catch ( err ) { | ||
| console.error( err.stdout ); | ||
| process.exitCode = 1; | ||
| } | ||
|
|
||
| /* eslint-enable no-console */ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| { | ||
| "extends": "../tsconfig.base.json", | ||
| "compilerOptions": { | ||
| "module": "commonjs", | ||
| "esModuleInterop": true, | ||
| "target": "ES6", | ||
| "lib": [ "ES6", "ES2020.string" ], | ||
| "rootDir": ".", | ||
| "declarationMap": false, | ||
|
|
||
| // We're not interested in the output, but we must generate | ||
| // something as part of a composite project. Use the | ||
| // ignored `.cache` file to hide tsbuildinfo and d.ts files. | ||
| "outDir": ".cache" | ||
| }, | ||
| "files": [ "./api-docs/update-api-docs.js" ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,7 +86,10 @@ | |
| "@storybook/addon-viewport": "5.3.2", | ||
| "@storybook/react": "5.3.2", | ||
| "@types/jest": "24.0.25", | ||
| "@types/lodash": "4.14.149", | ||
| "@types/qs": "6.9.1", | ||
| "@types/requestidlecallback": "0.3.1", | ||
| "@types/sprintf-js": "1.1.2", | ||
| "@wordpress/babel-plugin-import-jsx-pragma": "file:packages/babel-plugin-import-jsx-pragma", | ||
| "@wordpress/babel-plugin-makepot": "file:packages/babel-plugin-makepot", | ||
| "@wordpress/babel-preset-default": "file:packages/babel-preset-default", | ||
|
|
@@ -167,16 +170,18 @@ | |
| "sprintf-js": "1.1.1", | ||
| "style-loader": "1.0.0", | ||
| "stylelint-config-wordpress": "13.1.0", | ||
| "typescript": "3.8.3", | ||
| "uuid": "3.3.2", | ||
| "webpack": "4.42.0", | ||
| "worker-farm": "1.7.0" | ||
| }, | ||
| "scripts": { | ||
| "prebuild": "npm run check-engines", | ||
| "clean:packages": "rimraf \"./packages/*/@(build|build-module|build-style)\"", | ||
| "clean:package-types": "tsc --build --clean", | ||
|
There was a problem hiding this comment. Shouldn’t it be part of clean:packages that is executed as prebuild:packages? There was a problem hiding this comment. There's some conversation around this in the thread here: #18942 (comment) In summary, there's enormous benefit to having a warm TypeScript cache and I don't believe there's any reason to clean it except to remove outdated declaration files from packages before they will be published. In particular, we'll want to keep the caches warm for typechecking as part of lint staged. |
||
| "prebuild:packages": "npm run clean:packages && lerna run build", | ||
| "build:packages": "npm run build:package-types && node ./bin/packages/build.js", | ||
| "build:package-types": "tsc --build", | ||
| "build": "npm run build:packages && wp-scripts build", | ||
| "check-engines": "wp-scripts check-engines", | ||
| "check-licenses": "concurrently \"wp-scripts check-licenses --prod --gpl2\" \"wp-scripts check-licenses --dev\"", | ||
|
|
@@ -191,22 +196,21 @@ | |
| "fixtures:generate": "npm run fixtures:server-registered && cross-env GENERATE_MISSING_FIXTURES=y npm run test-unit", | ||
| "fixtures:regenerate": "npm run fixtures:clean && npm run fixtures:generate", | ||
| "format-js": "wp-scripts format-js", | ||
| "lint": "concurrently \"npm run lint-js\" \"npm run lint-pkg-json\" \"npm run lint-css\"", | ||
| "lint-js": "wp-scripts lint-js", | ||
| "lint-js:fix": "npm run lint-js -- --fix", | ||
| "lint-php": "npm run wp-env run composer run-script lint", | ||
| "lint-pkg-json": "wp-scripts lint-pkg-json . 'packages/*/package.json'", | ||
| "lint-css": "wp-scripts lint-style '**/*.scss'", | ||
| "lint-css:fix": "npm run lint-css -- --fix", | ||
sirreal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| "lint:md-js": "wp-scripts lint-md-js", | ||
| "lint:md-docs": "wp-scripts lint-md-docs", | ||
| "package-plugin": "./bin/build-plugin-zip.sh", | ||
| "pot-to-php": "./bin/pot-to-php.js", | ||
| "publish:check": "lerna updated", | ||
| "publish:dev": "npm run clean:package-types && npm run build:packages && lerna publish --dist-tag next", | ||
| "publish:legacy": "npm run clean:package-types && npm run build:packages && lerna publish --dist-tag legacy", | ||
| "publish:prod": "npm run clean:package-types && npm run build:packages && lerna publish", | ||
| "test": "npm run lint && npm run test-unit", | ||
| "test-e2e": "wp-scripts test-e2e --config packages/e2e-tests/jest.config.js", | ||
| "test-e2e:watch": "npm run test-e2e -- --watch", | ||
|
|
@@ -252,7 +256,8 @@ | |
| ], | ||
| "packages/**/*.js": [ | ||
| "node ./bin/api-docs/update-api-docs.js", | ||
| "node ./bin/api-docs/are-api-docs-unstaged.js", | ||
| "node ./bin/packages/lint-staged-typecheck.js" | ||
|
There was a problem hiding this comment. 901ce5d removed the custom build script and relies on a root I'm inclined to have the Once the TypeScript cache is warm, incremental builds are quick or instant. Running the entire type build has the benefit of detecting when type incompatibilities across packages are introduced, while the current version only detects that the changed package itself typechecks and issues between packages would surface later on CI when the types are compiled. @aduth Curious to hear your thoughts. There was a problem hiding this comment. I attempted to do this and it's quite difficult because npm run build:package-types "…/gutenberg/packages/a11y/src/index.js"
# tsc --build --verbose "…/gutenberg/packages/a11y/src/index.js"
# error TS6053: File '…/gutenberg/packages/a11y/src/index.js/tsconfig.json' not found.If Are we open to that? I'd prefer to handle that in its own PR. There was a problem hiding this comment.
Is the motivation to avoid maintaining the custom script, or because it may be faster to run a (warmed) full build than to typecheck individual package/files? I'm motivated to keep these pre-commits fast. I'm not overly concerned with the maintenance burden of a custom script, if it means we can have faster/more granular checks.
I agree it would be good to have more immediate feedback about potential issues. If it can be fast enough to not become a burden, then I like the idea. I'd also be content if at least this is considered in the full Travis build. Aside: Slow pre-commit has become a pet-peeve of mine lately (see #18840), in case that helps contextualize my thoughts in approaching this. There was a problem hiding this comment.
This is a motivation, I prefer offloading as much as possible directly to
This is not a concern. A warm build of everything will likely be almost identical to a warm typecheck of changed files. Only the necessary things would be rebuilt. The difference between a hot and cold cache is huge. The difference between a single package and all packages with a cold cache is also very large: # Cold cache all
npm run clean:packages; time npm run build:package-types
# real 0m37.884s
# user 0m46.316s
# sys 0m1.311s
# Cold cache only URL (random package)
npm run clean:packages; time npm run build:package-types -- packages/url
# real 0m5.567s
# user 0m8.644s
# sys 0m0.353s
# Hot cache all
npm run build:package-types; time npm run build:package-types
# real 0m0.515s
# user 0m0.356s
# sys 0m0.091sIf a package has changed, it will be rebuilt. If it's dependencies have not been built, they will be built first. Given that rebuilding an unchanged package with a warm cache is practically free, the difference between building a single package and building all packages should be insignificant. I agree these staged checks should be near immediate, and I don't feel too strongly about this. We might try to persist TypeScript caches longer. TypeScript does an excellent job of doing absolutely nothing very quickly if it doesn't need to. I suspect it's only important to clean these types files before publish, so perhaps There was a problem hiding this comment.
I've done this in 12302c0 where type cleaning is moved to publish scripts. There was a problem hiding this comment. 51195f3 proposes typechecking all modules on lint-staged. Feel free to play with it, but if you feel strongly we can roll this back. I don't feel strongly one way or the other. |
||
| ] | ||
| }, | ||
| "wp-env": { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep this script lint-staged, this could invoke
build:package-typesscript with-- ...changedPackages.I'd prefer to remove this entirely as noted: https://github.com/WordPress/gutenberg/pull/18942/files#r396317889