👷 reduce build boilerplate #3938
Conversation
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 8722954 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
BenoitZugmeyer
force-pushed
the
benoit/reduce-build-boilerplate
branch
2 times, most recently
from
b74ccc4 to
cc7a0e5
Compare
BenoitZugmeyer
force-pushed
the
benoit/reduce-build-boilerplate
branch
from
cc7a0e5 to
b3576cf
Compare
Bundles Sizes Evolution
🚀 CPU PerformancePending... 🧠 Memory PerformancePending... |
BenoitZugmeyer
force-pushed
the
benoit/reduce-build-boilerplate
branch
from
b3576cf to
de520ca
Compare
BenoitZugmeyer
force-pushed
the
benoit/reduce-build-boilerplate
branch
from
5ae0b2f to
2be9686
Compare
| // [1]: Quoting Lerna doc: "Lerna always uses npm to publish packages." | ||
| // https://lerna.js.org/docs/features/version-and-publish#from-package | ||
| const output = command`npm pack --dry-run --json`.withCurrentWorkingDirectory(packagePath).run() | ||
| const output = command`npm pack --ignore-scripts --dry-run --json`.withCurrentWorkingDirectory(packagePath).run() |
There was a problem hiding this comment.
Note: this is because npm pack runs the prepack npm script in the react package. Previously this script didn't output anything, but now it prints "Building modules...", breaking the JSON output.
Ignoring scripts is fine here, since the check-packages command is run after the packages are freshly built, so no need to build the react plugin again.
scripts/build/build-package.ts
Outdated
| const parsed = ts.parseJsonConfigFileContent( | ||
| { | ||
| extends: '../../tsconfig.base.json', | ||
| compilerOptions: { | ||
| baseUrl: '.', | ||
| declaration: true, | ||
| allowJs: true, | ||
| module, | ||
| rootDir: './src/', | ||
| outDir, | ||
| }, | ||
| include: ['./src'], | ||
| exclude: ['./src/**/*.spec.*', './src/**/*.specHelper.*'], | ||
| }, | ||
| ts.sys, | ||
| process.cwd(), | ||
| undefined, | ||
| 'tsconfig.json' // just used in messages | ||
| ) | ||
|
|
||
| const host = ts.createCompilerHost(parsed.options) | ||
| const program = ts.createProgram({ | ||
| rootNames: parsed.fileNames, | ||
| options: parsed.options, | ||
| host, | ||
| }) | ||
|
|
||
| const emitResult = program.emit() | ||
| const diagnostics = ts.getPreEmitDiagnostics(program).concat(emitResult.diagnostics) | ||
|
|
||
| if (diagnostics.length) { | ||
| const formatHost: ts.FormatDiagnosticsHost = { | ||
| getCanonicalFileName: (f) => f, | ||
| // eslint-disable-next-line @typescript-eslint/unbound-method | ||
| getCurrentDirectory: ts.sys.getCurrentDirectory, | ||
| getNewLine: () => ts.sys.newLine, | ||
| } | ||
| console.error(ts.formatDiagnosticsWithColorAndContext(diagnostics, formatHost)) | ||
| } | ||
|
|
||
| if (emitResult.emitSkipped) { | ||
| throw new Error('Failed to build package') | ||
| } |
There was a problem hiding this comment.
I don't love this TypeScript code, but at the same time I didn't find a better way to invoke TypeScript with a dynamic configuration (maybe with a temporary file that we put somewhere and feed to tsc... but I'm not sure it's better).
As I'm mentioning in the comment above, I hope we can move on from TypeScript for building packages at some point.
There was a problem hiding this comment.
To me it's fine but maybe you could clarify what it does with comments or split it into smaller functions with clear names?
| webpackBase({ | ||
| mode: 'development', | ||
| entry: `${packagePath}/src/entries/main.ts`, | ||
| filename: packageName === 'worker' ? 'worker.js' : `datadog-${packageName}.js`, |
There was a problem hiding this comment.
-
There is a little bit of duplication between the
build-package.tsanddev-server.ts. I believe this is acceptable as long as it's only a couple of lines. -
I don't love this
filenameexception forworker.js. Maybe we can unify filenames here, and always generate${packageName}.js(ex:rum.js,logs.js) or alwaysdatadog-${packageName}.js(ex:datadog-worker.js). Those paths are currently used in e2e tests and the devtools extension, so that's not as straightforward as it might seem. I think this is fine for now.
There was a problem hiding this comment.
yeah, having a more consistent naming would help having more generic scripts without having to hardcode names. let's keep that in mind, maybe for a mob session?
| webpackBase({ | ||
| mode: 'development', | ||
| entry: `${packagePath}/src/entries/main.ts`, | ||
| filename: packageName === 'worker' ? 'worker.js' : `datadog-${packageName}.js`, |
There was a problem hiding this comment.
yeah, having a more consistent naming would help having more generic scripts without having to hardcode names. let's keep that in mind, maybe for a mob session?
Motivation
I'm about to introduce a bunch of new packages. I want to reduce the amount of boilerplate we need to maintain within each package.
Changes
Introduce a new "build-package" script to unify how we build all packages. It supports two options,
--modulesand--bundleto either build modules or bundles (or both), following package needs.The
npm-run-alldependency has been removed. Packages are not built in parallel anymore, but that doesn't seem to impact the overall build time (at least on my machine):Before:
After:
Test instructions
Run
yarn build, check if everything is correctly built.Check if
yarn devis still working.Checklist