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

Hide all banners that tell you to change app URL #5172

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/app/src/cli/services/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import {installAppDependencies} from './dependencies.js'
import {DevConfig, DevProcesses, setupDevProcesses} from './dev/processes/setup-dev-processes.js'
import {frontAndBackendConfig} from './dev/processes/utils.js'
import {outputUpdateURLsResult, renderDev} from './dev/ui.js'
import {renderDev} from './dev/ui.js'
import {DeveloperPreviewController} from './dev/ui/components/Dev.js'
import {DevProcessFunction} from './dev/processes/types.js'
import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js'
Expand Down Expand Up @@ -116,10 +116,10 @@
await installAppDependencies(app)
}

const graphiqlPort = commandOptions.graphiqlPort || (await getAvailableTCPPort(ports.graphiql))

Check warning on line 119 in packages/app/src/cli/services/dev.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/services/dev.ts#L119

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
const {graphiqlKey} = commandOptions

if (graphiqlPort !== (commandOptions.graphiqlPort || ports.graphiql)) {

Check warning on line 122 in packages/app/src/cli/services/dev.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/services/dev.ts#L122

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
renderWarning({
headline: [
'A random port will be used for GraphiQL because',
Expand Down Expand Up @@ -223,7 +223,7 @@
scopesMessage(getAppScopesArray(localApp.configuration)),
'\n',
'Scopes in Partner Dashboard:',
scopesMessage(remoteAccess?.scopes?.split(',') || []),

Check warning on line 226 in packages/app/src/cli/services/dev.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/services/dev.ts#L226

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
],
nextSteps,
})
Expand Down Expand Up @@ -264,7 +264,7 @@
const newURLs = generatePartnersURLs(
network.proxyUrl,
webs.map(({configuration}) => configuration.auth_callback_path).find((path) => path),
isCurrentAppSchema(localApp.configuration) ? localApp.configuration.app_proxy : undefined,
localApp.configuration.app_proxy,
)
shouldUpdateURLs = await shouldOrPromptUpdateURLs({
currentURLs: network.currentUrls,
Expand All @@ -277,7 +277,6 @@
// When running dev app urls are pushed directly to API Client config instead of creating a new app version
// so current app version and API Client config will have diferent url values.
if (shouldUpdateURLs) await updateURLs(newURLs, apiKey, developerPlatformClient, localApp)
await outputUpdateURLsResult(shouldUpdateURLs, newURLs, remoteApp, localApp)
}
}
return shouldUpdateURLs
Expand All @@ -301,7 +300,7 @@
...frontEndOptions,
tunnelClient,
}),
getBackendPort() || backendConfig?.configuration.port || getAvailableTCPPort(),

Check warning on line 303 in packages/app/src/cli/services/dev.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/services/dev.ts#L303

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.

Check warning on line 303 in packages/app/src/cli/services/dev.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/services/dev.ts#L303

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
getURLs(remoteAppConfig),
])
const proxyUrl = usingLocalhost ? `${frontendUrl}:${proxyPort}` : frontendUrl
Expand Down
104 changes: 1 addition & 103 deletions packages/app/src/cli/services/dev/ui.test.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import {outputUpdateURLsResult, renderDev} from './ui.js'
import {renderDev} from './ui.js'
import {Dev} from './ui/components/Dev.js'
import {
testApp,
testDeveloperPlatformClient,
testFunctionExtension,
testOrganizationApp,
testThemeExtensions,
testUIExtension,
} from '../../models/app/app.test-data.js'
Expand Down Expand Up @@ -32,107 +31,6 @@ afterEach(() => {
mockAndCaptureOutput().clear()
})

describe('output', () => {
describe('outputUpdateURLsResult', () => {
const urls = {
applicationUrl: 'https://lala.cloudflare.io/',
redirectUrlWhitelist: ['https://lala.cloudflare.io/auth/callback'],
}

test('shows info about tunnel URL and links to Partners Dashboard when app is brand new', async () => {
// Given
const outputMock = mockAndCaptureOutput()
const localApp = await mockApp()

const remoteApp = testOrganizationApp({newApp: true})

// When
await outputUpdateURLsResult(true, urls, remoteApp, localApp)

// Then
expect(outputMock.output()).toMatchInlineSnapshot(`
"╭─ info ───────────────────────────────────────────────────────────────────────╮
│ │
│ For your convenience, we've given your app a default URL: │
│ https://lala.cloudflare.io/. │
│ │
│ You can update your app's URL anytime in the Partners Dashboard [1] But │
│ once your app is live, updating its URL will disrupt user access. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
[1] https://partners.shopify.com/1/apps/1/edit
"
`)
})

test('shows nothing when urls were updated', async () => {
// Given
const outputMock = mockAndCaptureOutput()
const localApp = await mockApp()

const remoteApp = testOrganizationApp({newApp: false})

// When
await outputUpdateURLsResult(true, urls, remoteApp, localApp)

// Then
expect(outputMock.output()).toEqual('')
})

test('shows how to update app urls on partners when app is not brand new, urls were not updated and app uses legacy config', async () => {
// Given
const outputMock = mockAndCaptureOutput()
const localApp = await mockApp()

const remoteApp = testOrganizationApp({newApp: false})

// When
await outputUpdateURLsResult(false, urls, remoteApp, localApp)

// Then
expect(outputMock.output()).toMatchInlineSnapshot(`
"╭─ info ───────────────────────────────────────────────────────────────────────╮
│ │
│ To make URL updates manually, you can add the following URLs as redirects │
│ in your Partners Dashboard [1]: │
│ │
│ │
│ • https://lala.cloudflare.io/auth/callback │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
[1] https://partners.shopify.com/1/apps/1/edit
"
`)
})

test('shows how to update app urls with config push when app is not brand new, urls were updated and app uses new config', async () => {
// Given
const outputMock = mockAndCaptureOutput()
const localApp = await mockApp(true)

const remoteApp = testOrganizationApp({newApp: false})

// When
await outputUpdateURLsResult(false, urls, remoteApp, localApp)

// Then
expect(outputMock.output()).toMatchInlineSnapshot(`
"╭─ info ───────────────────────────────────────────────────────────────────────╮
│ │
│ To update URLs manually, add the following URLs to │
│ shopify.app.staging.toml under auth > redirect_urls and run │
│ \`yarn shopify app config push --config=staging\` │
│ │
│ │
│ • https://lala.cloudflare.io/auth/callback │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
"
`)
})
})
})

describe('ui', () => {
describe('renderDev', () => {
test("doesn't use ink when terminal doesn't support TTY", async () => {
Expand Down
68 changes: 1 addition & 67 deletions packages/app/src/cli/services/dev/ui.tsx
Original file line number Diff line number Diff line change
@@ -1,67 +1,10 @@
import {PartnersURLs} from './urls.js'
import {Dev, DevProps} from './ui/components/Dev.js'
import {AppInterface, isCurrentAppSchema} from '../../models/app/app.js'
import {OrganizationApp} from '../../models/organization.js'
import {getAppConfigurationShorthand} from '../../models/app/loader.js'
import React from 'react'
import {partnersFqdn} from '@shopify/cli-kit/node/context/fqdn'
import {render, renderInfo} from '@shopify/cli-kit/node/ui'
import {basename} from '@shopify/cli-kit/node/path'
import {formatPackageManagerCommand} from '@shopify/cli-kit/node/output'
import {render} from '@shopify/cli-kit/node/ui'
import {terminalSupportsPrompting} from '@shopify/cli-kit/node/system'
import {isTruthy} from '@shopify/cli-kit/node/context/utilities'
import {isUnitTest} from '@shopify/cli-kit/node/context/local'

export async function outputUpdateURLsResult(
updated: boolean,
urls: PartnersURLs,
remoteApp: OrganizationApp,
localApp: AppInterface,
) {
const dashboardURL = await partnersURL(remoteApp.organizationId, remoteApp.id)
if (remoteApp.newApp) {
renderInfo({
headline: `For your convenience, we've given your app a default URL: ${urls.applicationUrl}.`,
body: [
"You can update your app's URL anytime in the",
dashboardURL,
'But once your app is live, updating its URL will disrupt user access.',
],
})
} else if (!updated) {
if (isCurrentAppSchema(localApp.configuration)) {
const fileName = basename(localApp.configuration.path)
const configName = getAppConfigurationShorthand(fileName)
const pushCommandArgs = configName ? [`--config=${configName}`] : []

renderInfo({
body: [
`To update URLs manually, add the following URLs to ${fileName} under auth > redirect_urls and run\n`,
{
command: formatPackageManagerCommand(
localApp.packageManager,
`shopify app config push`,
...pushCommandArgs,
),
},
'\n\n',
{list: {items: urls.redirectUrlWhitelist}},
],
})
} else {
renderInfo({
body: [
'To make URL updates manually, you can add the following URLs as redirects in your',
dashboardURL,
{char: ':'},
'\n\n',
{list: {items: urls.redirectUrlWhitelist}},
],
})
}
}
}

export async function renderDev({
processes,
previewUrl,
Expand Down Expand Up @@ -94,15 +37,6 @@ export async function renderDev({
}
}

async function partnersURL(organizationId: string, appId: string) {
return {
link: {
label: 'Partners Dashboard',
url: `https://${await partnersFqdn()}/${organizationId}/apps/${appId}/edit`,
},
}
}

async function renderDevNonInteractive({
processes,
app: {canEnablePreviewMode},
Expand Down
5 changes: 3 additions & 2 deletions packages/app/src/cli/services/dev/urls.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
import {UpdateURLsVariables} from '../../api/graphql/update_urls.js'
import {setCachedAppInfo} from '../local-storage.js'
import {patchAppConfigurationFile} from '../app/patch-app-configuration-file.js'
import {AppLinkedInterface} from '../../models/app/app.js'
import {beforeEach, describe, expect, vi, test} from 'vitest'
import {AbortError} from '@shopify/cli-kit/node/error'
import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp'
Expand Down Expand Up @@ -314,7 +315,7 @@ describe('shouldOrPromptUpdateURLs', () => {
currentURLs,
appDirectory: '/path',
apiKey: 'api-key',
localApp: testApp({configuration: {...DEFAULT_CONFIG, client_id: 'different'}}, 'current'),
localApp: testApp({configuration: {...DEFAULT_CONFIG, client_id: 'different'}}, 'current') as AppLinkedInterface,
}
vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)

Expand All @@ -334,7 +335,7 @@ describe('shouldOrPromptUpdateURLs', () => {
currentURLs,
appDirectory: '/path',
apiKey: 'api-key',
localApp,
localApp: localApp as AppLinkedInterface,
}
vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)

Expand Down
13 changes: 4 additions & 9 deletions packages/app/src/cli/services/dev/urls.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
import {updateURLsPrompt} from '../../prompts/dev.js'
import {
AppConfigurationInterface,
AppInterface,
CurrentAppConfiguration,
isCurrentAppSchema,
} from '../../models/app/app.js'
import {AppConfigurationInterface, AppLinkedInterface, CurrentAppConfiguration} from '../../models/app/app.js'
import {UpdateURLsSchema, UpdateURLsVariables} from '../../api/graphql/update_urls.js'
import {setCachedAppInfo} from '../local-storage.js'
import {AppConfigurationUsedByCli} from '../../models/extensions/specifications/types/app_config.js'
Expand Down Expand Up @@ -202,7 +197,7 @@
throw new AbortError(errors)
}

if (localApp && isCurrentAppSchema(localApp.configuration) && localApp.configuration.client_id === apiKey) {
if (localApp && localApp.configuration.client_id === apiKey) {
const patch = {
application_url: urls.applicationUrl,
auth: {
Expand Down Expand Up @@ -243,13 +238,13 @@
appDirectory: string
cachedUpdateURLs?: boolean
newApp?: boolean
localApp?: AppInterface
localApp?: AppLinkedInterface
apiKey: string
}

export async function shouldOrPromptUpdateURLs(options: ShouldOrPromptUpdateURLsOptions): Promise<boolean> {
if (options.localApp && options.localApp.configuration.client_id !== options.apiKey) return true
if (options.newApp || !terminalSupportsPrompting()) return true

Check warning on line 247 in packages/app/src/cli/services/dev/urls.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/services/dev/urls.ts#L247

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
let shouldUpdateURLs: boolean = options.cachedUpdateURLs === true

if (options.cachedUpdateURLs === undefined) {
Expand All @@ -258,7 +253,7 @@
options.currentURLs.redirectUrlWhitelist,
)

if (options.localApp && isCurrentAppSchema(options.localApp.configuration)) {
if (options.localApp) {
const localConfiguration = options.localApp.configuration
localConfiguration.build = {
...localConfiguration.build,
Expand Down
Loading