Add a default dev-session message for configuration-only extensions - #8350
Draft
amcaplan wants to merge 1 commit into
Draft
Add a default dev-session message for configuration-only extensions#8350amcaplan wants to merge 1 commit into
amcaplan wants to merge 1 commit into
Conversation
amcaplan
force-pushed
the
dev-session-message-config-only-extensions
branch
2 times, most recently
from
August 20, 2026 10:29
5cb60db to
364dbbe
Compare
Config-only extension modules say nothing during `shopify app dev`:
`logExtensionEvents` only runs inside `processEvents`, so at startup a
module that is live on the platform is never mentioned in the terminal.
Add a default `getDevSessionUpdateMessages` on `ExtensionInstance`,
gated on a capability predicate (no features, no deploy steps, no build
output, not app config) and on the first successful dev session. A
per-spec hook still wins, so any specification can override the copy.
The default lives on the instance rather than in the spec factories:
those pass `getDevSessionUpdateMessages` through unconditionally, so an
explicit `undefined` clobbers a factory-level default at
`{...defaults, ...spec}`.
`DevSessionResult` composes its success branch from the exported
`DevSessionUpdateStatus` union rather than restating the literals, so the
service and the spec hook can't drift apart. The logger takes the result
itself and decides which outcomes are worth speaking about, so the status
is never rewritten at a call site where it could disagree with the branch
it sits in. Only the narrow context reaches the specs.
Drop the `'aborted'` status while here. Nothing has constructed it since
4bc22d2 ("Only one app-preview update") removed the bundle-controller
abort check, so the branch handling it was unreachable. Deleting it lets
`DevSessionResult` reuse `DevSessionUpdateStatus` whole instead of
grafting a third literal onto it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
amcaplan
force-pushed
the
dev-session-message-config-only-extensions
branch
from
August 20, 2026 11:08
364dbbe to
5c7c980
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Config-only extension modules — the ones whose configuration is the whole extension, with nothing for the CLI to build, bundle or serve — say nothing at all during
shopify app dev.logExtensionEventsonly runs insideprocessEvents, so at startup these modules are invisible: you rundev, your module is in the manifest and live on the platform, and the terminal never mentions it.#8319 fixes this for one extension type (
analytics_app_events) by registering a local specification whose only purpose is to carry a single string. That works, but it means every future config-only, remote-only extension type needs its own local spec file for the same one line — which is exactly the coupling that remote-only specs exist to avoid.This PR solves the class instead of the instance: a default
getDevSessionUpdateMessagesfor config-only modules, overridable per specification.analytics_app_eventsgets its message with no local spec file at all, and so does the next one.WHAT is this pull request doing?
A default implementation on
ExtensionInstance.getDevSessionUpdateMessages, gated on a capability predicate and on the dev-session lifecycle:getDevSessionUpdateMessages, it is used unchanged. The three existing implementers (app_config_app_home,app_config_app_access,app_config_app_proxy) keep their current behaviour.appModuleFeatures, noclientStepsdeploy group, no build output, and not an app-config module. Read off existing lazy getters, so it stays correct when the platform response rewritesexperience/uidStrategyafter the factory has run.app-event-watcher.tsstamps every initial extension asEventType.Updated— so the signal isDevSessionResult.status === 'created', which by construction happens exactly once perdevrun.The default lives on the instance rather than in
createExtensionSpecificationon purpose.createConfigExtensionSpecificationandcreateContractBasedModuleSpecificationboth passgetDevSessionUpdateMessagesthrough unconditionally, so an explicitundefinedreaches{...defaults, ...spec}and clobbers any factory-level default — a default written there is a silent no-op for most specs. There is a test covering the override through all three factories.The hook also gains a second argument,
getDevSessionUpdateMessages(config, context), wherecontextis aDevSessionUpdateContextcarrying aDevSessionUpdateStatusof'created' | 'updated'.DevSessionResultreuses that union whole for its success branch, so the two can't drift: renaming a status now breaks the service at compile time.Responsibility splits across three layers.
handleDevSessionResultpasses itsDevSessionResultstraight to the logger — no reconstructed status literal, so there's no second copy that could disagree with the branch it was written in.DevSessionLogger.logExtensionUpdateMessagesdecides which outcomes are worth speaking about, returning early on the two error branches, which sits next to theEventType.Deletedskip it already owned. Errors already have a per-extension voice —logUserErrorsmaps eachUserErrorto its owning extension byuidand prefixes the line with that handle — so letting specs speak there too would be a second extension-prefixed channel on one event. Only the narrowDevSessionUpdateContextreaches the specs, so a status added later to the service's state machine doesn't silently widen what every spec has to handle — andspecification.tsstays free of any import back into the dev-session service, which would close a cycle throughapp-event-watcher.tsandextension-instance.ts.One drive-by removal: the
'aborted'status is gone. Nothing has constructed it since 4bc22d2 ("Only one app-preview update", May 2025) removed thenewBundleController.signal.abortedcheck, so thehandleDevSessionResultbranch that debug-logged it was unreachable — the statuses actually produced arecreated,updated,remote-errorandunknown-error. Deleting it letsDevSessionResultreuseDevSessionUpdateStatuswhole rather than grafting a third literal onto it, and leaves the logger's guard as an exact match on the two error branches. Behaviour is unchanged because the branch could not run. Happy to split this out into its own PR if you'd rather review it separately.The three existing implementations ignore the context and are unchanged — the only reason
app_config_app_access.test.tsandapp_config_app_proxy.test.tsappear in this diff is that they call the hook directly and now pass the new argument.Which specs this affects
editor_extension_collection,flow_action,flow_trigger,payments_extensionanalytics_app_eventschannel_config,order_attribution_config,tax_calculation,admin_link(deploy steps and/or build output), and all app-config modules (already summarised once asApp config updated)Open questions
Configuration acceptedsays only that the platform took the configuration. It deliberately claims nothing about local files, because the predicate admits modules where such a claim would be false:flow_actionandflow_triggerboth read a local schema file (loadSchemaFromPath, e.g.flow_action.ts:83). Anything more descriptive has to stay true for every module the predicate admits, now and as the set grows. Owners of the four affected local specs should also confirm they want the line at all.flow_actionprintsConfiguration acceptedwhile acheckout_ui_extensionin the same app prints nothing. The rule is "nothing is built for this module", not "this module is inert" — buildable modules are already visible through their build output and preview URL, whereas these had no representation indevoutput at all. That's the intended reading, but it is the boundary most likely to be contested, so it should be an explicit decision rather than a side effect.features === ['localization']. That's a deliberate consequence of the predicate rather than an oversight; widening it would start printing for modules that do have local output.createContractBasedModuleSpecificationpassesuidStrategy: spec.uidStrategyunconditionally, so omitting it clobbers the computed default at{...defaults, ...spec}withundefined,buildHandle's switch falls through, andconstantize(undefined)throws. No production caller hits it —createRemoteOnlySpecificationalways passes a strategy — so it's dormant, and fixing it means touching the merge line this PR is specifically designed to avoid. Worth its own PR. It's also a live demonstration, on a second property, of why the default here lives on the instance rather than in the factory.meta_datawithdescriptionanddocumentation_links(23 registrations inshop/worldhave it). Sourcing dev-session copy from the platform instead of hardcoding it in the CLI is the better long-term shape, and would let each extension team own its own string. Out of scope here, which is CLI-side copy only.How to test your changes?
shopify app devon an app with a config-only module (e.g. a Flow action, or ananalytics_app_eventsmodule).✅ Ready, watching for changes, the module printsConfiguration acceptedunder its own handle.Extension changed/✅ Updated dev preview on …and does not repeatConfiguration accepted.Post-release steps
None.
Checklist
minor, changeset added