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

Adapting app to run on CLI 3 #1

Merged
merged 3 commits into from
Jun 10, 2022
Merged

Adapting app to run on CLI 3 #1

merged 3 commits into from
Jun 10, 2022

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Jun 9, 2022

This takes the previous CLI 2 instance of a rails app and adapts it to CLI 3.

To 🎩:

  1. yarn create @shopify/app --template https://github.com/Shopify/shopify-app-template-ruby#run_on_cli_3
  2. dev clone shopify_app (or dev cd)
  3. git checkout allow_custom_oauth_routes
  4. pwd
  5. Go back to the app folder, edit web/Gemfile, add , path: '<result from above>' to the line for shopify_app
  6. cd web
  7. bundle
  8. bin/rails db:migrate
  9. cd ..
  10. yarn dev

Steps 2-5 will not be necessary once we publish a new version of shopify_app with Shopify/shopify_app#1445

@paulomarg
Copy link
Contributor Author

paulomarg commented Jun 9, 2022

Dumping a few things will be pending once this is merged (most are pretty small):

  • Setting up github files / CI / CODEOWNERS, PR templates, CLA, etc.
  • Making sure webhooks work as expected
  • Making sure the app can be uninstalled and reinstalled without a db reset
  • Adding the /api/products-count endpoint
  • GDPR webhook handling
  • Billing
  • Setting this app up to run in production mode
  • Setting up a README / hosting instructions
  • Adding it as an option when creating a new app with the CLI

@paulomarg paulomarg requested a review from teddyhwang June 9, 2022 18:52
@teddyhwang
Copy link

Shouldn't we have shopify_app generate these files?

@paulomarg
Copy link
Contributor Author

paulomarg commented Jun 10, 2022

I think there's an argument for both cases here. All of the changes I made were to take the app generated by shopify_app and replace the default FE it outputs with the react app (and adjust routes accordingly).

Since the gem can be used to generate any app (including Shopify ones), I'm not sure we want the gem itself to produce something that uses the react app / CLI 3 structure by default - that would mean:

  • Cloning the submodule internally and adding all of those files, which is awkward because:
    • If you re-generate things, it would go over each file asking if you want to replace it (unless we blindly copy it on top of what's there or fully ignore the folder which would lead to stuff breaking)
    • The generators kind of become a mini-version of CLI 2 since they have to handle cloning and stuff
  • Whenever the generators are run, we'd try to re-create the FE app - unless we add different generators each time a version comes out that changes files.

If we assume re-generating would be out of the question, we'd still be left with a scenario where shopify_app would be in charge of updating the FE app, which I'm not sure is a problem or not - would people ever want to update their FE only?

@teddyhwang
Copy link

I think the generator would only be used to update this repo whenever changes are made to the rails structure. if we're fine to manually update these files on this repo every time there's a new rails version then I'm good with this approach.

would people ever want to update their FE only?

we need to make sure that we continue to position this as a starter template and not a framework that gets regular updates. the role of the template completes for the user after it has been scaffolded so with that in mind, is it better to keep this information inside of shopify_app?

@paulomarg paulomarg merged commit 3597586 into main Jun 10, 2022
@paulomarg paulomarg deleted the run_on_cli_3 branch June 10, 2022 14:55
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.

3 participants