Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Add "shopify app" commands #1650

Merged
merged 22 commits into from
Nov 8, 2021
Merged

Add "shopify app" commands #1650

merged 22 commits into from
Nov 8, 2021

Conversation

pepicrft
Copy link
Contributor

@pepicrft pepicrft commented Oct 18, 2021

Fixes https://github.com/Shopify/shopify-cli-planning/issues/59

WHY are these changes introduced?

I'm iterating on the taxonomy of the app commands to group them under shopify app and remove the need for passing the type, which is information that can be obtained from the project's directory.

WHAT is this pull request doing?

This PR moves the sub-commands under the shopify app command. As part of this effort, I've removed messages and business logic duplication across the commands (there were many copy-pasting patterns)‚ and extracted business logics into services that allow easily reusing the business logic across commands.

Note moving all the commands in one shot resulted in many changes and thus a large PR that might make the review a bit hard. Because I didn't touch the business logic, and there were already some tests in place that bring some confidence, I refrained from doing a breakdown. If you think that'll help you with the review, let me know and I can figure out the process for gradual migration.

How to test your changes?

I recommend creating an app of each type and using each of the commands with the app. All of them should work as expected. Pay close attention to the output messages too to make sure the merging of messages from all the project types was done right.

Post-release steps

The counterpart PR for updating the dev.shopify.com documentation should get deployed too.

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).
  • I've included any post-release steps in the section above.

@pepicrft pepicrft requested a review from a team October 18, 2021 10:57
@pepicrft pepicrft self-assigned this Oct 18, 2021
@pepicrft pepicrft requested review from hannachen and jesalerno84 and removed request for a team October 18, 2021 10:57
@pepicrft pepicrft force-pushed the shopify-app branch 3 times, most recently from 3e60db5 to 6a6c899 Compare October 19, 2021 14:53
@pepicrft pepicrft marked this pull request as ready for review October 20, 2021 11:18
@pepicrft pepicrft requested a review from a team October 20, 2021 11:18
@pepicrft pepicrft requested a review from a team as a code owner October 20, 2021 11:18
@hannachen
Copy link

This might just be something on my side, but I'm not always able to use newly added sub commands, in particular app whenever it's introduced. Are there any additional steps I need to take to get it to work?

Screen Shot 2021-10-26 at 4 23 26 PM

@pepicrft
Copy link
Contributor Author

This might just be something on my side, but I'm not always able to use newly added sub commands, in particular app whenever it's introduced. Are there any additional steps I need to take to get it to work?

That's a bug (good catch). Even if the help doesn't work, you should be able to do shopify app create node | rails | php. I'll fix that and push a commit with the fix.

@pepicrft
Copy link
Contributor Author

@hannachen shopify app create --help should work if you pull the latest changes.

@jesalerno84
Copy link
Contributor

Screen Shot 2021-10-27 at 2 50 57 PM

I don't know if it is the same issue as you are having @hannachen - but I cannot get any of the app commands to run?

@hannachen
Copy link

Good news... shopify app create --help is now displaying the correct help context...!

Bad news... the other commands still don't seem to be working for me. I ran dev up before testing... might I be missing a step? Is this the correct way to test the branch? 🤔 ./bin/shopify [command]:

Screen Shot 2021-10-27 at 3 41 55 PM

@pepicrft
Copy link
Contributor Author

My bad! Would you mind trying again?

@jesalerno84
Copy link
Contributor

My bad! Would you mind trying again?

That worked. 🎩 ing now

Copy link
Contributor

@jesalerno84 jesalerno84 left a comment

Choose a reason for hiding this comment

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

🎩 = ✅

@hannachen
Copy link

Is there an error message/help for incomplete commands? The CLI seem to be breaking for me when I only enter shopify app create with the app type missing:

Screen Shot 2021-10-28 at 4 23 10 PM

Traceback (most recent call last):
	23: from ./bin/shopify:47:in `'
	22: from ./bin/shopify:36:in `block in '
	21: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/error_handler.rb:21:in `call'
	20: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/error_handler.rb:75:in `handle_abort'
	19: from ./bin/shopify:37:in `block (2 levels) in '
	18: from /Users/hanna/src/github.com/Shopify/shopify-cli/lib/shopify_cli/core/entry_point.rb:23:in `call'
	17: from /Users/hanna/src/github.com/Shopify/shopify-cli/lib/shopify_cli/core/monorail.rb:30:in `log'
	16: from /Users/hanna/src/github.com/Shopify/shopify-cli/lib/shopify_cli/core/entry_point.rb:24:in `block in call'
	15: from /Users/hanna/src/github.com/Shopify/shopify-cli/lib/shopify_cli/core/executor.rb:15:in `call'
	14: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/executor.rb:43:in `with_traps'
	13: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/executor.rb:55:in `twrap'
	12: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/executor.rb:44:in `block in with_traps'
	11: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/executor.rb:55:in `twrap'
	10: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/executor.rb:45:in `block (2 levels) in with_traps'
	 9: from /Users/hanna/src/github.com/Shopify/shopify-cli/lib/shopify_cli/core/executor.rb:16:in `block in call'
	 8: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/executor.rb:35:in `with_logging'
	 7: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-ui/lib/cli/ui.rb:176:in `log_output_to'
	 6: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/executor.rb:36:in `block in with_logging'
	 5: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-ui/lib/cli/ui/stdout_router.rb:169:in `with_id'
	 4: from /Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/executor.rb:37:in `block (2 levels) in with_logging'
	 3: from /Users/hanna/src/github.com/Shopify/shopify-cli/lib/shopify_cli/core/executor.rb:17:in `block (2 levels) in call'
	 2: from /Users/hanna/src/github.com/Shopify/shopify-cli/lib/shopify_cli/command.rb:24:in `call'
	 1: from /Users/hanna/src/github.com/Shopify/shopify-cli/lib/shopify_cli/command.rb:30:in `call'
/Users/hanna/src/github.com/Shopify/shopify-cli/vendor/deps/cli-kit/lib/cli/kit/base_command.rb:41:in `call': NotImplementedError (NotImplementedError)

@pepicrft
Copy link
Contributor Author

Is there an error message/help for incomplete commands? The CLI seem to be breaking for me when I only enter shopify app create with the app type missing:

It shouldn't. I pushed a fix for that.

Copy link

@hannachen hannachen left a comment

Choose a reason for hiding this comment

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

🎩 👍
Tested the commands locally, using shopify app create for all three project types with shopify app serve. It's great to not have to specify the project type when serving an app!

@pepicrft pepicrft merged commit c24a7fd into main Nov 8, 2021
@pepicrft pepicrft deleted the shopify-app branch November 8, 2021 15:24
@pepicrft pepicrft mentioned this pull request Nov 15, 2021
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems November 15, 2021 11:48 Inactive
@gonzaloriestra gonzaloriestra mentioned this pull request Feb 2, 2022
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants