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.

Sign up for GitHub

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

Jump to bottom

Fix Dev Console issues #5444

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix Dev Console issues #5444

wants to merge 4 commits into from

Conversation

vividviolet
Copy link
Member

@vividviolet vividviolet commented Feb 20, 2025 •
edited
Loading

WHY are these changes introduced?

This PR addresses 2 issues.

Mismatched ids

When connecting to the Dev Console, sometimes updates are lost due to mismatched devUUID after the ids are fetched from Partners. Here's how this happens:

  1. Dev process is started with a devUUID generated from the handle, ex. dev-abc
  2. GraphQL query is made to fetch the extensions on prod and updateExtensionUUIDS is called with an updated uuid of real-uuid
  3. A toml change is made which triggers reloadApp to be called
  4. Extensions instances are re-created with a devUUID generated from the handle, ex. dev-abc. Since we don't call updateExtensionUUIDS again the real uuid gets lost
  5. A code change is made to the extension which which triggers an update event for the extension dev-abc
  6. The Dev Console ignores the update events since dev-abc doesn't match real-uuid.

Out of date extension instances

The Dev Console holds onto old extension instances that are never updated when the app reloads which causes a 404 error when visiting a target that was updated after the dev process has started. Here's how this happens:

  1. Dev process is started with a ui extension with targets set to [{target: 'admin.product-details.block.render']
  2. A toml change is made to update the target to [{target: 'admin.catalog-details.block.render'] which triggers reloadApp to be called.
  3. Extensions instances are re-created with the new targets but the Dev Console still holds onto the extension instances from initialization
  4. Clicking on the deep link to the new targets results in a 404 because hasExtensionPointTarget is called on the old instance which doesn't have the new target

WHAT is this pull request doing?

  • Reuse devUUID and uid when reloading the app
  • Update dev console's extension instances when app reloads

Before

Screen.Recording.2025-02-21.at.8.44.13.PM.mov

After

Screen.Recording.2025-02-21.at.8.41.35.PM.mov

How to test your changes?

  1. Check out this PR (or use the snapshot build) to dev and app with a Admin Block UI extension
  2. Open the Dev Console and click the preview link and verify that it redirects correctly
  3. Change the target the UI extension's toml to something different and also update the code to render the new target
  4. Reopen the Dev Console and click on the preview link for the new target
  5. Verify that it redirects successfully

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

vividviolet
vividviolet commented Feb 20, 2025
View reviewed changes
packages/app/src/cli/models/app/loader.ts
@@ -480,8 +480,12 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS
entryPath = await this.findEntryPath(directory, specification)
}

const previousExtension = this.previousApp?.allExtensions.find((extension) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@isaacroldan any ideas how to test this? I couldn't figure out how to write a test for reloadApp. I tried writing one but it just aborts with the "Error loading the app, please check your app configuration." error.

vividviolet
vividviolet commented Feb 20, 2025
View reviewed changes
packages/app/src/cli/services/dev/extension.ts
@@ -126,17 +126,26 @@ export async function devUIExtensions(options: ExtensionDevOptions): Promise<voi
const bundlePath = payloadOptions.appWatcher.buildOutputPath
const payloadStoreRawPayload = await getExtensionsPayloadStoreRawPayload(payloadOptions, bundlePath)
const payloadStore = new ExtensionsPayloadStore(payloadStoreRawPayload, payloadOptions)
let extensions = payloadOptions.extensions

const getExtensions = () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of having a getExtensions we could mutate payloadOptions.extensions directly. Not sure if that's better though because it hides away the details. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like having an explicit "getExtensions" method 👌

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 20, 2025 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
75.73% (-0.18% 🔻)
 9073/11981
🟡 Branches
70.94% (-0.12% 🔻)
 4424/6236
🟡 Functions
75.51% (-0.01% 🔻)
 2380/3152
🟡 Lines
76.24% (-0.22% 🔻)
 8567/11237
Show new covered files 🐣
St.❔
 File Statements Branches Functions Lines
🔴
... / identity-token-validation.ts
 0% 0% 0% 0%
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟢
... / ui_extension.ts
 98.08%
86.67% (-0.83% 🔻)
 100% 98.08%
🟡
... / dev.ts
78% (-2.77% 🔻)
65.22% (-1.45% 🔻)
77.78% (-4.58% 🔻)
75% (-3.26% 🔻)
🟢
... / usePollAppLogs.ts
93.75% (-4.43% 🔻)
96% (+10.81% 🔼)
81.82% (-18.18% 🔻)
93.48% (-4.63% 🔻)
🟢
... / select-store.ts
83.67% (-0.64% 🔻)
81.82% (-1.52% 🔻)
 71.43%
84.78% (-2.45% 🔻)
🟢
... / app-event-watcher.ts
95.18% (-1.2% 🔻)
86.49% (-2.7% 🔻)
 95.45% 100%
🔴
... / app-management-client.ts
35.45% (-0.24% 🔻)
 27.66% 35.05%
34.41% (-0.26% 🔻)
🔴
... / partners-client.ts
26.62% (-0.52% 🔻)
 37.5% 17.54%
26.32% (-0.55% 🔻)
🔴
... / graphql.ts
5.88% (-60.78% 🔻)
0% (-55.56% 🔻)
25% (-41.67% 🔻)
5.88% (-59.83% 🔻)
🔴
... / ui.tsx
40.91% (-1.52% 🔻)
 35.29%
40.63% (-3.13% 🔻)
41.27% (-1.59% 🔻)
🟢
... / graphql.ts
84% (+0.67% 🔼)
37.5% (-19.64% 🔻)
100% (+11.11% 🔼)
83.33% (+0.48% 🔼)
🔴
... / api.ts
53.97% (-12.28% 🔻)
39.77% (-12.28% 🔻)
53.49% (-13.18% 🔻)
54.75% (-13.46% 🔻)

Test suite run success

2048 tests passing in 913 suites.

Report generated by 🧪jest coverage report action from 066edbe

@vividviolet
Reuse devUUID and uid when reloading the app
80a53da 
Update dev console's extension instances when app reloads
@vividviolet vividviolet force-pushed the fix-out-dated-dev-console branch from df03c94 to 80a53da Compare February 21, 2025 02:09
isaacroldan
isaacroldan reviewed Feb 21, 2025
View reviewed changes
packages/app/src/cli/models/app/loader.ts Outdated
const extensionInstance = new ExtensionInstance({
configuration,
configuration: {...configuration, uid: previousExtension?.uid, devUUID: previousExtension?.devUUID},
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't include this as part of the config, as the config is typed and the devUUID doesn't belong there.

Let's just update the extensionInstance after creating it

@vividviolet vividviolet marked this pull request as ready for review February 22, 2025 01:23
@vividviolet vividviolet requested review from a team as code owners February 22, 2025 01:23
@vividviolet
Copy link
Member Author

/snapit

@github-actions GitHub Actions
Copy link
Contributor

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g @shopify/[email protected]

Tip

If you get an ETARGET error, install it with NPM and the flag --@shopify:registry=https://registry.npmjs.org

Caution

After installing, validate the version by running just shopify in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isaacroldan isaacroldan isaacroldan left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla-needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vividviolet @isaacroldan