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): Setup command which uses an npm package #8920

Merged
merged 15 commits into from
Sep 14, 2023
Merged

Conversation

Josh-Walker-GM
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM commented Jul 15, 2023

Problem
We want to allow users to run arbitary setup commands that come from npm packages. This is similar to the functionality of npx or dlx but we want to allow additional redwood specific behaviour. Specifically for now we want to ensure version compatibility between the setup package and the redwood version of the user's project.

Notes
I published the package @joshgmw/rw-setup-example as a means of testing the functionality in this PR.

Changes

  1. Adds a yarn rw setup package <npm-package> command which will:
    1. Find the provided package on npm and check version compatibility
    2. Run yarn dlx with that package at the user project root

Notes
The UI of this isn't amazing but I don't really know what our consistent design for the CLI is.

The naming of the command or where it sits in the command hierarchy is something I'm happy to change if we feel the further need to.

@Josh-Walker-GM Josh-Walker-GM added the release:feature This PR introduces a new feature label Jul 15, 2023
@Josh-Walker-GM Josh-Walker-GM added this to the v6.0.0 milestone Jul 15, 2023
@jtoar jtoar modified the milestones: v6.0.0, next-release Jul 27, 2023
@jtoar jtoar modified the milestones: next-release, v6.1.0 Aug 12, 2023
@Josh-Walker-GM Josh-Walker-GM removed the request for review from thedavidprice August 26, 2023 01:01
@Josh-Walker-GM Josh-Walker-GM marked this pull request as ready for review August 29, 2023 00:17
@Josh-Walker-GM Josh-Walker-GM added the fixture-ok Override the test project fixture check label Aug 29, 2023
@Josh-Walker-GM
Copy link
Collaborator Author

I marked this as fixture-ok because I think it is. I tried to run the rebuild fixture command locally but I don't think it's working all that well right now as it hit an error completely unrelated to any of my changes.

@jtoar jtoar modified the milestones: next-release, v6.2.0 Sep 2, 2023
@Tobbe
Copy link
Member

Tobbe commented Sep 5, 2023

I tried to run the rebuild fixture command locally but I don't think it's working all that well right now as it hit an error completely unrelated to any of my changes.

I rebuilt the fixture yesterday. It failed on the first try. But running again it got all the way through

@Josh-Walker-GM
Copy link
Collaborator Author

Okay @jtoar I think I've addressed the points you raised when we talked about this. It should be in a state for review now.

There's less of the uncaught erroring out but I can't say the UI of this is great right now. I think this is likely just difficult because I feel we lack a consistent style or approach to CLI output at the moment.

When it comes to testing it out the package I published has a few different depencency ranges which you can see from the packument here: https://registry.npmjs.org/@joshgmw/rw-setup-example. That and manipulating your package.json @redwoodjs/core version should hopefully let you test the various cases.

Happy to iterate on this further if you feel things are quite right with it.

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.

@Josh-Walker-GM awesome work! thanks for providing a published package to test it out with. and love the tests! here's what i've got so far. will probably review one more time.

this kind of error seems unrecoverable right? if we continue, it's just going to fail. maybe we should just exit and skip the prompt? you can't run something that's not published:

image

packages/cli/src/commands/setup/package/package.js Outdated Show resolved Hide resolved
packages/cli/src/commands/setup/package/package.js Outdated Show resolved Hide resolved
packages/cli/src/commands/setup/package/package.js Outdated Show resolved Hide resolved
packages/cli/src/commands/setup/package/package.js Outdated Show resolved Hide resolved
packages/cli/src/commands/setup/package/package.js Outdated Show resolved Hide resolved
@Josh-Walker-GM
Copy link
Collaborator Author

@jtoar I've updated based on your feedback thanks! I was happy to swiftly exit when the package doesn't have the version or tag requested - as you said it'll error out a few seconds later anyway.

@jtoar jtoar enabled auto-merge (squash) September 14, 2023 23:04
@jtoar jtoar merged commit 66a0e4a into main Sep 14, 2023
29 checks passed
@jtoar jtoar deleted the jgmw-cli/arb-setup branch September 14, 2023 23:54
jtoar pushed a commit that referenced this pull request Sep 15, 2023
**Problem**
We want to allow users to run arbitary setup commands that come from npm
packages. This is similar to the functionality of `npx` or `dlx` but we
want to allow additional redwood specific behaviour. Specifically for
now we want to ensure version compatibility between the setup package
and the redwood version of the user's project.

**Notes**
I published the package `@joshgmw/rw-setup-example` as a means of
testing the functionality in this PR.

**Changes**
1. Adds a `yarn rw setup package <npm-package>` command which will:
   1. Find the provided package on npm and check version compatibility
   2. Run `yarn dlx` with that package at the user project root

**Notes**
The UI of this isn't amazing but I don't really know what our consistent
design for the CLI is.

The naming of the command or where it sits in the command hierarchy is
something I'm happy to change if we feel the further need to.
@jtoar jtoar modified the milestones: next-release, v6.3.0 Sep 16, 2023
jtoar pushed a commit that referenced this pull request Sep 16, 2023
**Problem**
I left in a console.log in #8920 which should have been removed before
merging

**Changes**
1. Removes the offending console.log
jtoar pushed a commit that referenced this pull request Sep 16, 2023
**Problem**
I left in a console.log in #8920 which should have been removed before
merging

**Changes**
1. Removes the offending console.log
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

3 participants