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

Make developer console support create/delete extensions mid-dev #5118

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

Conversation

isaacroldan
Copy link
Contributor

@isaacroldan isaacroldan commented Dec 17, 2024

WHY are these changes introduced?

Adds support for handling extension creation and deletion events in the developer console during consistent-dev.

Fixes https://github.com/Shopify/develop-app-outer-loop/issues/1198

WHAT is this pull request doing?

  • Implements handling for Created and Deleted extension events
  • Adds new methods to ExtensionsPayloadStore for adding and removing extensions
  • Updates the event handler to maintain the extensions list in the payload store options

KNOWN ISSUES:

  • When removing an extension, you need to manually refresh the dev-console.

How to test your changes?

  1. Start dev for an app with UI extensions
  2. Create a new UI extension and verify it appears in the dev console (and that the deeplink works)
  3. Delete a UI extension and verify it's removed from the dev console (after refreshing)
  4. Modify an existing extension and confirm updates are still working

Measuring impact

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

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

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Contributor

github-actions bot commented Dec 17, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.18% (-0.13% 🔻)
8841/11759
🟡 Branches
70.48% (-0.1% 🔻)
4290/6087
🟡 Functions
75.11% (-0.1% 🔻)
2315/3082
🟡 Lines
75.73% (-0.12% 🔻)
8358/11036
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / extension.ts
54.55% (-31.17% 🔻)
0%
60% (-15% 🔻)
56.25% (-29.46% 🔻)
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% (-2.7% 🔻)
90.48% 98.61%
🟢
... / store.ts
86.54% (-13.46% 🔻)
87.5% (-12.5% 🔻)
86.36% (-13.64% 🔻)
87.23% (-12.77% 🔻)
🟢
... / ConcurrentOutput.tsx
98.36% (-1.64% 🔻)
88% (-4% 🔻)
100%
98.33% (-1.67% 🔻)

Test suite run success

1994 tests passing in 902 suites.

Report generated by 🧪jest coverage report action from 147046b

this.handle = this.buildHandle()
this.devUUID = `dev-${nonRandomUUID(this.handle)}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this we ensure that the devUUID is always the same for the same extension. Allowing us to reload it if we need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that the handle can change (right?) so it's not 100% foolproof but it's close enough that it should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't change during dev though, which solves the issue of "reloading" the app.

In any case, this is mostly for ui-extensions, and we need to rethink why ui-extensions need a devUUID in the first place. Will probably do it in early january.

this.handle = this.buildHandle()
this.devUUID = `dev-${nonRandomUUID(this.handle)}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that the handle can change (right?) so it's not 100% foolproof but it's close enough that it should be fine.

Comment on lines +119 to +120
// NOTE: Always use `payloadOptions`, never `options` directly. This way we can mutate `payloadOptions` without
// affecting the original `options` object and we only need to care about `payloadOptions` in this function.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we could make options of type Readonly<ExtensionDevOptions> but there are also downsides as Readonly is a somewhat weak guarantee and might make us think code is safe when it really isn't. Not a mandatory change, but something to consider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I just forced all other functions to expect a ExtensionsPayloadStoreOptions, that way i'm sure we are using payloadOptions

packages/app/src/cli/services/dev/extension.ts Outdated Show resolved Hide resolved
@isaacroldan isaacroldan marked this pull request as ready for review December 18, 2024 17:00
@isaacroldan isaacroldan requested a review from a team as a code owner December 18, 2024 17:00

This comment has been minimized.

isaacroldan and others added 17 commits January 3, 2025 16:53
If this is the first time deploying and there is no UID, the
autogenerated UID won't yet be found in `module.configuration`.

All singleton shopify.app.toml-based extensions don't need assets, so
even though they should technically use module.handle here, it basically
doesn't matter. And for all other extension types, `module.uid` is the
correct value to use here.
Just pass `SHOPIFY_APP_TEMPLATES_JSON_PATH=/path/to/your/file` and it
will draw from there instead of pulling from the live URL.
@isaacroldan isaacroldan requested a review from a team as a code owner January 3, 2025 15:54
@isaacroldan isaacroldan requested a review from amcaplan January 3, 2025 15:57
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.

7 participants