CLI responsiveness

[peterp]

The CLI is a bit slow. It takes ~1150ms to run redwood --help. I believe speed is part of DX and we should try to aim for ~300ms response time in the CLI.

We're also using yargs at Snaplet and the approach we've taken is to separate the handlers (the logic that the CLI runs) from the structure that dictates the commands the CLI exposes. We do this by asynchronously importing the code in the handler.

@jtoar @dac09 I would love some input on the approach I'm taking before fully committing to it.

Closes #6027

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 3b13aba. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 14 targets

• cli run test --concurrency 2 -- --colors --maxWorkers 2
• cli run test --concurrency 2 -- --colors --maxWorkers 2
• cli run test --concurrency 2 -- --colors --maxWorkers 2
• cli run test --concurrency 2 -- --colors --maxWorkers 2
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build
• cli run build

---

Sent with 💌 from NxCloud.

[netlify]

✅ Deploy Preview for redwoodjs-docs canceled.

Name	Link
🔨 Latest commit	3b13aba
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62e259364bc2460008f84994

[peterp]

hyperfine --warmup 3 'node node_modules/@redwoodjs/cli/dist/index.js --help'
Benchmark 1: node node_modules/@redwoodjs/cli/dist/index.js --help
  Time (mean ± σ):     791.3 ms ±   2.8 ms    [User: 777.2 ms, System: 167.5 ms]
  Range (min … max):   787.6 ms … 795.8 ms    10 runs

The last two biggest candidates are the build and serve command

[jtoar]

@peterp love the approach so far! The way you're doing the imports here gives us as much lazy evaluation as we can get out of yargs I think. The command, description, and builder all need to be available right away, but not the handler. (Accordingly, we should make sure the builders are clean. There’s a few commands that import some things. Probably nothing too crazy, but still.)

A somewhat-related concern I have that you've already made a few moves on is all the CLI's dependencies, and what's going on in lib. Writing a simple isTypeScriptProject helper instead of importing @redwoodjs/structure is a solid approach. (I really wish we could bundle the helpers we need into CLI and not list any dependencies at all, but that would require 1) decoupling the way code is written 2) overhauling the build process.) But did you have more thoughts on how you wanted to tackle dependencies and lib?

And my main question is: how can I help more concretely? Are there commands I can take from you? (build or serve? Both, or more?)

Here's a few other things that occur to me. All of these could be handled separately, but just wanted to share:

• There's some bugs around dotenv loading I wonder if we could fix at the same time. Note that this probably a symptom of what I did to get yarn 3 working
  • Prisma loads .env, processes differently from RWJS #5657
• We use a few lodash functions, but list the whole package as a dependency. We could shed some weight there
  • Along the same lines, we should make sure prompts isn't too heavy. if it is, we can switch to enquirer
• I'd be happy to add a test for CLI responsiveness to CI. Also, this package needs way more tests. That should be easier to organize with the handlers separated out
• I’d be happy to update the README after this is all over. Or I can do it as a part of this PR

One more thing: execa. I think we should try to standardize the way we use it as much as possible. I'm also down to remove it, but it's probably harder to replace than I think.

[jtoar]

Mind explaining this one? Are you trying to make it so that we just import path functions and not the kitchen sink with internal?

[peterp]

@jtoar I think what we're trying to achieve is "tree shaking," but we're doing that manually here!

For a long running process I guess it's fine to pay the upfront cost of importing all this JavaScript, but for something like a CLI it's gonna hurt. I think we should use package.json "exports" to expose a light version of "@redwoodjs/internal" that we can use is middleware and for the builder parts of the CLI.

[dac09]

This is exactly what I was thinking of doing. I think we should export these individual files, rather than everything from index.js - which is part of the problem with internal (and structure... but that's a harder one)

[peterp]

@jtoar I think it would be great if you could help with the build and serve command. Maybe we could merge this approach to yield off merge conflicts? and then I'll create a loom video that explains to the process to discover the "hot paths" and what we're trying to achieve for the other commands.

I think we could also look at tree-shaking or bundling as a possible approach to solve this automatically, but I do not have a bunch of confidence in those.

(I really wish we could bundle the helpers we need into CLI and not list any dependencies at all, but that would require 1)

I think your intuition is totally correct here.

But did you have more thoughts on how you wanted to tackle dependencies and lib?

I was thinking that most of the functionality in lib/index.js should be placed in separate files, so we only import what we need at the time we need it. (mostly getPaths)

[dac09]

@jtoar I think it would be great if you could help with the build and serve command. Maybe we could merge this approach to yield off merge conflicts? and then I'll create a loom video that explains to the process to discover the "hot paths" and what we're trying to achieve for the other commands.

This would be incredibly helpful @peterp - this is something I've struggled with. On another PR - Tobbe mentioned using yarn nx graph - that gives us a high level overview of how our packages depend on each other.

[jtoar]

LGTM! @dac09 I reverted the debugging change we made to the imports back to the way Peter had it (from dist). The problem was just that most of the tests were mocking @redwoodjs/internal, not @redwoodjs/internal/dist/paths. After making that change and mocking the new functions in lib/project, things went back to passing.

If this looks good to you, we can merge this then focus on making legitimate entry points to @redwoodjs/internal, either using "exports" or another style—whatever we all settle on after doing some research and experimentation.

[dac09]

HODL ✋ - found a bug in build - prerender fails with "getTasks" not found. Not sure why CI isn't catching it - @jtoar nx caching perhaps?

[dac09]

Fixed the bug

[jtoar]

HODL ✋ - found a bug in build - prerender fails with "getTasks" not found. Not sure why CI isn't catching it - @jtoar nx caching perhaps?

Looked into it, and it turns out that CI isn't catching it cause we don't test for it. We run yarn rw build --no-prerender:

redwood/.github/workflows/ci.yml

Lines 196 to 199 in 20bf7c1

	- name: Run `rw build` without prerender
	run: |
	yarn rw build --no-prerender
	working-directory: ${{ steps.setup_test_project.outputs.test_project_path }}

getTasks in yarn rw build is only invoked if it prerenders too:

redwood/packages/cli/src/commands/buildHandler.js

Lines 117 to 132 in 6ec4599

	prerender && {
	title: 'Prerendering Web...',
	task: async () => {
	const prerenderRoutes = detectPrerenderRoutes()
	if (prerenderRoutes.length === 0) {
	return `You have not marked any "prerender" in your ${terminalLink(
	'Routes',
	'file://' + rwjsPaths.web.routes
	)}.`
	}
	return new Listr(await getPrerenderTasks(), {
	renderer: verbose && VerboseRenderer,
	concurrent: true, // Re-use prerender tasks, but run them in parallel to speed things up
	})
	},
	},

But why doesn't the yarn rw build unit test fail? Turns out it also doesn't test the lines where getTasks is called:

redwood/packages/cli/src/commands/__tests__/build.test.js

Lines 59 to 62 in 6ec4599

	// Run prerendering task, but expect failure,
	// because `detectPrerenderRoutes` is empty.
	const x = await Listr.mock.calls[0][0][2].task()
	expect(x.startsWith('You have not marked any "prerender" in your Routes'))