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

Add deprecated flag warning to build #609

Merged
merged 3 commits into from
Mar 2, 2023

Conversation

cartogram
Copy link
Contributor

@cartogram cartogram commented Mar 2, 2023

WHY are these changes introduced?

This PR adds a helper that we can add to the commands whose flags have changed from going from v1 to v2. Currently the CLI will fail if an unsupported flag is passed without much indication of why. For example:

Screenshot 2023-03-02 at 06 14 06

I saw the above example come up in Discord.

WHAT is this pull request doing?

There are a few ways we can address this, but I chose to create a small helper we can add to the Command's flag object. In this PR I've added it to some of the deprecated build flags.

HOW to test your changes?

Run the CLI locally with a flag from H1 (ie: --entry).

Post-merge steps

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes

@cartogram cartogram requested a review from frandiox March 2, 2023 05:16
@github-actions

This comment has been minimized.

@cartogram
Copy link
Contributor Author

Screenshot 2023-03-02 at 06 29 08

This solution forces the banners to render individually, if we wanted a different experience, we might want to add a catch to the command. For example:

const DEPRECATED_FLAGS = [...]

 async catch(error: Error & {args?: string[]}) {
   if (error?.args?.some((flag) => DEPRECATED_FLAGS.includes(flag))) {
      renderInfo({
        headline: `The ${colors.bold(
          error.args.join(' '),
        )} flags are deprecated and will be removed in a future version of Shopify CLI.`,
      });

    throw error;
  }

My feeling is that 90% of the time users will hit this error when moving from H1 to H2 and not updating the build script in their package json (which has a --entry flag).

Copy link
Contributor

@frandiox frandiox left a comment

Choose a reason for hiding this comment

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

Great idea!

Can we add this to the dev command as well for the --entry flag?

I think it's fine if we render the banners separately, it's just a warning that shouldn't come up often.

Copy link
Contributor

@lordofthecactus lordofthecactus left a comment

Choose a reason for hiding this comment

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

@cartogram
Copy link
Contributor Author

@pepicrft @matteodepalo maybe something like this should make it's way into the cli-kit 🤔 I could see this being a common pattern.

@cartogram cartogram merged commit 4443a2b into 2023-01 Mar 2, 2023
@cartogram cartogram deleted the @cartogram/deprecated-flag-helper branch March 2, 2023 17:45
@github-actions github-actions bot mentioned this pull request Mar 2, 2023
@matteodepalo
Copy link

Interesting! Definitely something that we could add to cli-kit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants