Skip to content
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

feat(cli): Lazy install storybook #8454

Merged
merged 11 commits into from
Jun 1, 2023
Merged

feat(cli): Lazy install storybook #8454

merged 11 commits into from
Jun 1, 2023

Conversation

Josh-Walker-GM
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM commented May 31, 2023

Motivation
We enabled experimental support for CLI commands via npm packages. This should allow us to extract out some of redwood's heavier CLI commands to be lazy installed when they are first used. This will allow a leaner initial install size. Storybook is reasonably large and not used by every user so this is a good first test case.

Changes

  • Extracts out storybook command into a dedicated package @redwoodjs/cli-storybook
  • Removed storybook dependencies from @redwoodjs/testing
  • Added @redwoodjs/cli-storybook to the default TOML for experimental.cli.plugins - this will mean the command is known to the CLI even though the package is not installed
  • The TOML defaults to have experimental.cli.autoInstall = true so that packages are automatically installed when they are needed.
  • Added a default command-cache.json file inside the CRWA templates which will allow the CLI to lazy load this new @redwoodjs/cli-storybook package without needing to load (and install it) on the first arbitrary CLI invocation.

Changes to CLI plugins
This PR also made some small changes to the CLI plugin support. Specifically:

  • Plugins do not need to specify a version but they can. If not specified the package will attempt to be installed at the same version as the redwood CLI. This for the time being allows easier installation of @redwoodjs/* packages.

Issues

  1. I don't yet have any idea how to test this without having it in canary...
  2. Will need to check that installing storybook into the root workspace's dev dependencies is appropriate.

@Josh-Walker-GM Josh-Walker-GM added the release:feature This PR introduces a new feature label May 31, 2023
@Josh-Walker-GM Josh-Walker-GM added the fixture-ok Override the test project fixture check label May 31, 2023
@replay-io
Copy link

replay-io bot commented May 31, 2023

19 replays were recorded for a3a80e5.

image 0 Failed
image 19 Passed

View test run on Replay ↗︎

@jtoar
Copy link
Contributor

jtoar commented Jun 1, 2023

@Josh-Walker-GM haven't reviewed yet, but quick reaction to:

  1. Will need to check that installing storybook into the root workspace's dev dependencies is appropriate

I'd think that they would only need to be in the web workspace's dev dependencies, but I know there may be something to consider with the CLI "bin proxies" I think I understand now

Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the code changes look good; most suggestions I have would be stylistic, so approved, and we can discuss realtime. I don't have good ideas for testing. We need a version of @redwoodjs/testing that doesn't have the storybook dependencies, and it feels like we just need to publish a version of it that doesn't have them.

import fg from 'fast-glob'

// Get source files
const sourceFiles = fg.sync(['./src/**/*.ts'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice makes sense

packages/cli-packages/storybook/src/commands/storybook.ts Outdated Show resolved Hide resolved
packages/cli-packages/storybook/src/types.ts Outdated Show resolved Hide resolved
@jtoar jtoar marked this pull request as ready for review June 1, 2023 21:35
@jtoar jtoar merged commit 8fb0f86 into main Jun 1, 2023
25 checks passed
@jtoar jtoar deleted the jgmw-cli/storybook-package branch June 1, 2023 22:08
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jun 1, 2023
@jtoar jtoar modified the milestones: next-release, v6.0.0 Jun 1, 2023
jtoar added a commit that referenced this pull request Aug 29, 2023
…8572)

This PR makes the data migrate CLI command (`yarn rw data-migrate`) a
plugin like #8454 did for
storybook. But this PR goes a little further, also adding a bin that
runs the data migrate's `up` command. I don't think it makes sense for
all of Redwood's CLI commands to do this, but it does for some, and data
migrate is one since it's an important part of Docker deploys.

- This PR extracts the Babel config out of `@redwoodjs/internal` into
its own package, `@redwoodjs/babel-config`. If I didn't do this,
`@redwoodjs/cli-data-migrate` would depend on `@redwoodjs/internal`
because it needs the `registerApiSideBabelHook` function, which would
make it's package size unacceptable
- Even without internal, the new package has an unfortunate dependency
on `@redwoodjs/structure` that I'm not going to try to refactor in this
PR
- Just noting that `@redwoodjs/structure` doesn't depend on any other
Redwood packages besides `@redwoodjs/project-config`, so I'm sure we
could make it smaller if we took the time to focus on it
- The Babel config in `@redwoodjs/internal` had a dependency on the
`typescript` package that I had to refactor as well
- The Babel config needs a lot of TLC, but strictly speaking it'd be
breaking so I'm holding off on that for now again. I'll open another PR
after this one that has those changes
jtoar added a commit that referenced this pull request Sep 2, 2023
…8572)

This PR makes the data migrate CLI command (`yarn rw data-migrate`) a
plugin like #8454 did for
storybook. But this PR goes a little further, also adding a bin that
runs the data migrate's `up` command. I don't think it makes sense for
all of Redwood's CLI commands to do this, but it does for some, and data
migrate is one since it's an important part of Docker deploys.

- This PR extracts the Babel config out of `@redwoodjs/internal` into
its own package, `@redwoodjs/babel-config`. If I didn't do this,
`@redwoodjs/cli-data-migrate` would depend on `@redwoodjs/internal`
because it needs the `registerApiSideBabelHook` function, which would
make it's package size unacceptable
- Even without internal, the new package has an unfortunate dependency
on `@redwoodjs/structure` that I'm not going to try to refactor in this
PR
- Just noting that `@redwoodjs/structure` doesn't depend on any other
Redwood packages besides `@redwoodjs/project-config`, so I'm sure we
could make it smaller if we took the time to focus on it
- The Babel config in `@redwoodjs/internal` had a dependency on the
`typescript` package that I had to refactor as well
- The Babel config needs a lot of TLC, but strictly speaking it'd be
breaking so I'm holding off on that for now again. I'll open another PR
after this one that has those changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixture-ok Override the test project fixture check release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants