New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(CLI): build with esbuild #10323
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thedavidprice
changed the title
chore(CLI): bulid with esbuild
chore(CLI): build with esbuild
Mar 27, 2024
jtoar
pushed a commit
that referenced
this pull request
Mar 29, 2024
As @jtoar pointed out in #10323 we load and depend on additional packages as a result of building with our babel build config. This PR switches out the `@redwoodjs/telemetry` package to use esbuild over babel for building itself. The changes here are: 1. Remove babel config and dependencies 2. Build with esbuild instead, adding `tsx` and `typescript` dev dependencies 3. Build types too. We didn't previously do that, impact on package size vs usefulness? The resulting file changes in the dist output are that the existing files change and that each file now also has a `.d.ts` and `d.ts.map` companion. Although this does reduce the number of files/packages loaded when you run `yarn rw dev --help` it doesn't come with a noticeable performance improvement. Still nice not to load stuff that we don't actually need.
thedavidprice
pushed a commit
that referenced
this pull request
Mar 29, 2024
I started working on `yarn rw dev` for RSCs by looking at the CLI. I wanted to start by getting a wholistic picture of everything that happens. There's some options `yarn rw dev` takes that really shouldn't be advertised anymore because they're only relevant to Webpack. I thought about conditionally showing them by importing some stuff to detect `vite.config.ts`, etc. But you don't want to import too much in those command module definition files because yargs loads them all, even just for `yarn rw --help`. All the heavy stuff is in the command module's handler because you're actually running the code at that point so it's ok to pay the cost. Well what is required by the time we load `./packages/cli/src/commands/dev.js`? (I.e. `yarn rw dev --help`) (The way to read this is: `"package name": number of files imported from said package`, at least according to `require.cache`.) <details> <summary>Click for the whole list (104 in total) ```json { "core-js-pure": 439, "semver": 89, "@babel/types": 82, "core-js": 69, "@babel/core": 49, "//": "..." } ``` </summary> ```json { "//": "..." "@opentelemetry/api": 47, "fs-extra": 27, "fast-glob": 25, "@babel/runtime-corejs3": 23, "uuid": 16, "@redwoodjs/cli": 15, "@iarna/toml": 14, "@nodelib/fs.walk": 9, "@nodelib/fs.scandir": 9, "braces": 7, "execa": 7, "picomatch": 6, "cross-spawn": 6, "@nodelib/fs.stat": 5, "graceful-fs": 4, "prettier": 4, "human-signals": 4, "yargs": 3, "@babel/helper-validator-identifier": 3, "debug": 3, "supports-color": 3, "color-convert": 3, "jsonfile": 2, "has-flag": 2, "chalk": 2, "dotenv": 2, "lru-cache": 2, "yallist": 2, "@redwoodjs/telemetry": 2, "isexe": 2, "signal-exit": 2, "get-stream": 2, "cli-boxes": 2, "@redwoodjs/core": 1, "universalify": 1, "y18n": 1, "yargs-parser": 1, "cliui": 1, "string-width": 1, "strip-ansi": 1, "ansi-regex": 1, "is-fullwidth-code-point": 1, "emoji-regex": 1, "wrap-ansi": 1, "ansi-styles": 1, "escalade": 1, "get-caller-file": 1, "require-directory": 1, "@redwoodjs/cli-helpers": 1, "to-fast-properties": 1, "@babel/helper-string-parser": 1, "gensync": 1, "ms": 1, "listr2": 1, "eventemitter3": 1, "colorette": 1, "rfdc": 1, "color-name": 1, "@redwoodjs/project-config": 1, "deepmerge": 1, "string-env-interpolation": 1, "glob-parent": 1, "is-glob": 1, "is-extglob": 1, "micromatch": 1, "fill-range": 1, "to-regex-range": 1, "is-number": 1, "merge2": 1, "run-parallel": 1, "queue-microtask": 1, "fastq": 1, "reusify": 1, "terminal-link": 1, "ansi-escapes": 1, "supports-hyperlinks": 1, "which": 1, "path-key": 1, "shebang-command": 1, "shebang-regex": 1, "strip-final-newline": 1, "npm-run-path": 1, "onetime": 1, "mimic-fn": 1, "is-stream": 1, "merge-stream": 1, "pascalcase": 1, "boxen": 1, "widest-line": 1, "camelcase": 1, "ansi-align": 1, "decamelize": 1, "lodash": 1, "param-case": 1, "tslib": 1, "dot-case": 1, "no-case": 1, "lower-case": 1, "pluralize": 1 } ``` </details> It just makes me sad that most of the CLI's imports are Babel or core-js when they're not really necessary at this point, especially for the CLI. (Yeah yeah, the files that are imported may not cost much to import so importing prettier may add up to more than all those packages combined etc. But we need prettier and we don't need all those packages.) The solution is to do what we've done for so many packages now: build it with a different tool. We use esbuild. The CLI was a bit trickier than other packages because it needs many files that don't end in `.ts/.js` (all those generator templates). So I just had to add an extra step. Have at the extra step. Tested by checking dist into git, using `find`, bemoaning Windows paths, and CI. But does this actually work? Somewhat for sure. Here's the before-after: <img width="1614" alt="image" src="https://github.com/redwoodjs/redwood/assets/32992335/f40b3baf-be91-4192-9eb2-8cce736d1f81"> But is it actually faster? Rudimentary hyperfine testing says so: ```bash # Before redwood-app % hyperfine --warmup 3 --runs 5 'yarn rw dev --help' Benchmark 1: yarn rw dev --help Time (mean ± σ): 460.4 ms ± 7.5 ms [User: 580.9 ms, System: 72.2 ms] Range (min … max): 453.4 ms … 471.3 ms 5 runs # After redwood-app % hyperfine --warmup 3 --runs 5 'yarn rw dev --help' Benchmark 1: yarn rw dev --help Time (mean ± σ): 406.6 ms ± 4.1 ms [User: 525.5 ms, System: 56.3 ms] Range (min … max): 403.3 ms … 413.2 ms 5 runs ``` Other errata: - Some directories have a `.keep` file in them, but they have content, so that doesn't need to be there anymore - The less files that we put in src that aren't shipped to the user the better. That goes for `vitest.codemods.setup.ts` - Some templates were `*.xyzTemplate`. Changed them to `*.xyz.template`. It's OK to be consistent
thedavidprice
pushed a commit
that referenced
this pull request
Mar 29, 2024
As @jtoar pointed out in #10323 we load and depend on additional packages as a result of building with our babel build config. This PR switches out the `@redwoodjs/telemetry` package to use esbuild over babel for building itself. The changes here are: 1. Remove babel config and dependencies 2. Build with esbuild instead, adding `tsx` and `typescript` dev dependencies 3. Build types too. We didn't previously do that, impact on package size vs usefulness? The resulting file changes in the dist output are that the existing files change and that each file now also has a `.d.ts` and `d.ts.map` companion. Although this does reduce the number of files/packages loaded when you run `yarn rw dev --help` it doesn't come with a noticeable performance improvement. Still nice not to load stuff that we don't actually need.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I started working on
yarn rw dev
for RSCs by looking at the CLI. I wanted to start by getting a wholistic picture of everything that happens.There's some options
yarn rw dev
takes that really shouldn't be advertised anymore because they're only relevant to Webpack. I thought about conditionally showing them by importing some stuff to detectvite.config.ts
, etc. But you don't want to import too much in those command module definition files because yargs loads them all, even just foryarn rw --help
. All the heavy stuff is in the command module's handler because you're actually running the code at that point so it's ok to pay the cost.Well what is required by the time we load
./packages/cli/src/commands/dev.js
? (I.e.yarn rw dev --help
)(The way to read this is:
"package name": number of files imported from said package
, at least according torequire.cache
.)Click for the whole list (104 in total)
It just makes me sad that most of the CLI's imports are Babel or core-js when they're not really necessary at this point, especially for the CLI. (Yeah yeah, the files that are imported may not cost much to import so importing prettier may add up to more than all those packages combined etc. But we need prettier and we don't need all those packages.) The solution is to do what we've done for so many packages now: build it with a different tool. We use esbuild.
The CLI was a bit trickier than other packages because it needs many files that don't end in
.ts/.js
(all those generator templates). So I just had to add an extra step. Have at the extra step.Tested by checking dist into git, using
find
, bemoaning Windows paths, and CI.But does this actually work? Somewhat for sure. Here's the before-after:
But is it actually faster? Rudimentary hyperfine testing says so:
Other errata:
.keep
file in them, but they have content, so that doesn't need to be there anymorevitest.codemods.setup.ts
*.xyzTemplate
. Changed them to*.xyz.template
. It's OK to be consistent