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 6 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.14% (-0.08% 🔻)
8813/11728
🟡 Branches
70.49% (-0.06% 🔻)
4287/6082
🟡 Functions
74.93% (-0.1% 🔻)
2304/3075
🟡 Lines
75.7% (-0.06% 🔻)
8331/11006
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / context.ts
70.75%
60% (-2.07% 🔻)
73.91% 73.33%
🔴
... / extension.ts
51.43% (-34.29% 🔻)
0%
60% (-15% 🔻)
52.94% (-32.77% 🔻)
🟢
... / 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% 🔻)

Test suite run success

1989 tests passing in 898 suites.

Report generated by 🧪jest coverage report action from 303d315

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

Comment on lines +149 to +152
outputDebug(
`Failed to build a cart URL for your checkout extension. Use the --checkout-cart-url flag to set a fixed URL.`,
payloadOptions.stdout,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this is a unique error case because in the general case, if something fails inside buildCartURLIfNeeded, we can just crash dev and no harm done. I'm curious why we wouldn't:

  1. Crash dev in this case
  2. Keep going and just not provide the convenience link

Also, why would it not work? I guess just because:

  1. Connection to the store fails
  2. There are no products to test on

These feel like rare cases. 1 would be very odd given that we have other polling processes, and 2 would be odd not to crash or at least warn that no products exist. Note that the error message is raised inside fetchProductVariant so I'd expect we should at least display it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure how to handle this case, there are two main possibilities why this might fail:

  • There are no products
  • You don't have permissions to access the admin API to fetch products

I don't think we should stop dev, as everything else works, the extension will work, what won't work is the "deeplink" to it... if you create a cart manually you'll see the extension.

Maybe instead of showing anything we should change the deeplink to take you to your store main page if there is no product here?

What do you think?

@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
Copy link
Contributor

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

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.

2 participants