-
Notifications
You must be signed in to change notification settings - Fork 145
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
Hide all banners that tell you to change app URL #5172
Hide all banners that tell you to change app URL #5172
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success1996 tests passing in 902 suites. Report generated by 🧪jest coverage report action from a622479 |
This comment has been minimized.
This comment has been minimized.
1207bd4
to
cdee477
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I was not able to tophat. When I follow the testing script for partners I never see the message. Here's my output
cli git:(jg/hide-partners-banner-app-url-when-using-appmgmtapi) p shopify app init --path=/Users/mitch/test-apps/
Note that p is an alias for pnpm activated by shadowenv in the CLI repository
> @0.0.0 shopify /Users/mitch/src/github.com/Shopify/cli
> nx build cli && node packages/cli/bin/dev.js "app" "init" "--path=/Users/mitch/test-apps/"
Debugger attached.
✔ 10/10 dependent project tasks succeeded [10 read from cache]
Hint: you can run the command with --verbose to see the full dependent project outputs
———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
> nx run cli:build [existing outputs match the cache, left as is]
> pnpm tsc -b ./tsconfig.build.json
Debugger attached.
Waiting for the debugger to disconnect...
———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
NX Successfully ran target build for project cli and 10 tasks it depends on (273ms)
Nx read the output from the cache instead of running the command for 11 out of 11 tasks.
Waiting for the debugger to disconnect...
Debugger attached.
To run this command, log in to Shopify.
User verification code: LVKJ-ZTQK
👉 Press any key to open the login page on your browser
Opened link to start the auth process: https://accounts.shopify.com/activate-with-code?device_code%5Buser_code%5D=LVKJ-ZTQK
✔ Logged in.
? Get started building your app:
✔ Build an extension-only app
? Which organization is this work for?
✔ Mitch Dickinson Partner Biz
? Create this project as a new app on Shopify?
✔ Yes, create it as a new app
? App name:
✔ jan9-test
╭─ info ───────────────────────────────────────────────────────────────────────╮
│ │
│ Initializing project with `pnpm` │
│ Use the `--package-manager` flag to select a different package manager. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
╭─ success ────────────────────────────────────────────────────────────────────╮
│ │
│ jan9-test is ready for you to build! │
│ │
│ Next steps │
│ • Run `cd jan9-test` │
│ • For extensions, run `shopify app generate extension` │
│ • To see your app, run `shopify app dev` │
│ │
│ Reference │
│ • Shopify docs │
│ • For an overview of commands, run `shopify app --help` │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Waiting for the debugger to disconnect...
➜ cli git:(jg/hide-partners-banner-app-url-when-using-appmgmtapi) p shopify app dev --path=/Users/mitch/test-apps/jan9-test --reset
Note that p is an alias for pnpm activated by shadowenv in the CLI repository
> @0.0.0 shopify /Users/mitch/src/github.com/Shopify/cli
> nx build cli && node packages/cli/bin/dev.js "app" "dev" "--path=/Users/mitch/test-apps/jan9-test" "--reset"
Debugger attached.
✔ 10/10 dependent project tasks succeeded [10 read from cache]
Hint: you can run the command with --verbose to see the full dependent project outputs
———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
> nx run cli:build [existing outputs match the cache, left as is]
> pnpm tsc -b ./tsconfig.build.json
Debugger attached.
Waiting for the debugger to disconnect...
———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
NX Successfully ran target build for project cli and 10 tasks it depends on (282ms)
Nx read the output from the cache instead of running the command for 11 out of 11 tasks.
Waiting for the debugger to disconnect...
Debugger attached.
? Which organization is this work for?
✔ Mitch Dickinson Partner Biz
? Create this project as a new app on Shopify?
✔ No, connect it to an existing app
? Which existing app is this for?
✔ jan9-test (shopify.app.toml)
╭─ success ────────────────────────────────────────────────────────────────────╮
│ │
│ shopify.app.toml is now linked to "jan9-test" on Shopify │
│ │
│ Using shopify.app.toml as your default config. │
│ │
│ Next steps │
│ • Make updates to shopify.app.toml in your local project │
│ • To upload your config, run `shopify app deploy` │
│ │
│ Reference │
│ • App configuration │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
? Which store would you like to use to view your project?
✔ mitch-dev-store
╭─ info ───────────────────────────────────────────────────────────────────────╮
│ │
│ Using shopify.app.toml for default values: │
│ │
│ • Org: Mitch Dickinson Partner Biz │
│ • App: jan9-test │
│ • Dev store: mitch-dev-store.myshopify.com │
│ • Update URLs: Not yet configured │
│ │
│ You can pass `--reset` to your command to reset your app configuration. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
07:00:52 │ graphiql │ GraphiQL server started on port 3457
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
› Press g │ open GraphiQL (Admin API) in your browser
› Press p │ preview in your browser
› Press q │ quit
Preview URL:
https://mitch-dev-store.myshopify.com/admin/oauth/redirect_from_cli?client_id=5d4505f3c23f26aa67009ad78688c3ca
GraphiQL URL: http://localhost:3457/graphiql
I'm actually getting the same result! I suspect the |
@amcaplan @MitchDickinson can you try again but making sure you select to create a new app?
Here you should choose this option: |
Still not working for me: https://share.descript.com/view/FbFodrR8Rjg |
Okay, last requirement you must pick a Remix app template. I'd imagine an extension-only app doesn't need an App URL so they don't show the banner or set the |
Ah, that makes sense! I created a new app but forgot it has to be the Remix template! |
Honestly, I wonder if we should just get rid of this warning entirely. Now that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as this works. Added a couple comments, and I think the most important bit is that maybe we don't need this warning at all?
…en using App Management API
cdee477
to
1547704
Compare
I removed the warning altogether since I think that was Isaac's ultimate suggestion. |
4411167
to
0b7a1f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Deleted code is best code 👍
0b7a1f3
to
55fe914
Compare
) { | ||
const dashboardURL = await partnersURL(remoteApp.organizationId, remoteApp.id) | ||
if (remoteApp.newApp) { | ||
export async function outputUpdateURLsResult(updated: boolean, urls: PartnersURLs, localApp: AppInterface) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remoteApp is no longer needed to check since we are now only looking if users wanted us to update their TOML's app URLs for them during dev. If not, just let them know they can do it themselves with the tunneled URLs.
d777777
to
a5eeda5
Compare
a5eeda5
to
39ca8ca
Compare
44d71fd
to
bcc170f
Compare
bcc170f
to
a622479
Compare
Just to confirm, we are OK with not showing any message at all related to the URLs (whether they were updated or not) right? |
Do we need UX weigh in here? |
Got @nickwesselman 's blessing here! |
WHAT is this pull request doing?
Fixes https://github.com/Shopify/develop-app-inner-loop/issues/2348
Removes this app url change banner since it is no longer needed by Developer Dashboard apps or apps using consistent dev
Additionally,
isCurrentAppSchema
is always true now, so we remove some of the mentions of it from the files we are touching.How to test your changes?
From within the CLI folder:
Try not using the App Management API first and confirm partners dashboard app url update banner doesn't show
p shopify app init
p shopify app dev --path ./name-of-your-app --reset
Now try using the App Management API and confirm partners dashboard app url update banner doesn't show
USE_APP_MANAGEMENT_API=1 p shopify app dev --path ./name-of-your-app --reset
Confirm that the app config push banner is gone:
automatically_update_urls_on_dev
defined-
USE_APP_MANAGEMENT_API=1 p shopify app dev --path ./name-of-your-app
Checklist