refactor(!): improved type-narrowing when working with action settings (relates #133) - #164
refactor(!): improved type-narrowing when working with action settings (relates #133)#164GeekyEggo wants to merge 13 commits into
Conversation
🦋 Changeset detectedLatest commit: 4bd7a5d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Refactors the plugin action type system to better enforce an Action<T>’s settings shape at setSettings call sites, and propagates explicit JsonObject typing through internal stores/controllers where the concrete settings type is unknown.
Changes:
- Tightens
Action.setSettingsto accept onlyT(removing the independent generic that allowed unrelated settings objects). - Removes default type parameters from
Action,DialAction, andKeyAction, and updates internal APIs/stores to useJsonObjectexplicitly where needed. - Updates action-service tests and adds changesets to ship the breaking type changes as a major version.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/plugin/src/plugin/ui.ts | Updates stored/returned action types to use `DialAction |
| packages/plugin/src/plugin/devices/device.ts | Tightens Device.actions iterator type to explicitly carry JsonObject settings. |
| packages/plugin/src/plugin/actions/store.ts | Makes the action store explicitly store `DialAction |
| packages/plugin/src/plugin/actions/key.ts | Removes default settings generic to require explicit settings typing. |
| packages/plugin/src/plugin/actions/dial.ts | Removes default settings generic to require explicit settings typing. |
| packages/plugin/src/plugin/actions/action.ts | Tightens setSettings to accept only T, improving type narrowing at call sites. |
| packages/plugin/src/plugin/actions/tests/service.test.ts | Updates casts to include explicit settings generic arguments. |
| .changeset/wild-bikes-vanish.md | Declares a major release note for the setSettings type-narrowing change. |
| .changeset/free-oranges-lose.md | Declares a major release note for required explicit settings typing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/plugin/src/plugin/actions/action.ts:112
setSettingsis now correctly constrained toT, butgetSettingsstill exposes an unconstrained generic (getSettings<U extends JsonObject = T>()), so callers can still bypassAction<T>by explicitly providing an unrelatedU. If the goal is to makeAction<T>actually enforce settings shape (per #133),getSettingsshould also be constrained to returnPromise<T>without a caller-supplied generic.
* @param value Settings to persist.
* @returns `Promise` resolved when the settings are sent to Stream Deck.
*/
public setSettings(value: T): Promise<void> {
settingsCache.delete(this.id);
packages/plugin/src/plugin/actions/action.ts:27
- Removing the default type parameter from
Action<T>makes bareActiontype references invalid. There is at least one remaining usage (packages/plugin/src/plugin/actions/__tests__/action.test.tshaslet action!: Action;) that will fail TypeScript compilation unless updated to include a concrete type argument (e.g.Action<JsonObject>).
export class Action<T extends JsonObject> extends ActionContext {
…ng, improve JSDocs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
Suppressed comments (8)
packages/plugin/src/plugin/actions/singleton-action.ts:111
setTitleis implemented byKeyActionandDialAction, notActionBase, so this newly targeted API link cannot resolve to a real member.
* Occurs when the user updates an action's title settings in the Stream Deck application. See also {@link ActionBase.setTitle}.
packages/plugin/src/plugin/actions/dial.ts:58
setFeedbackis declared onDialAction, notActionBase, so this API link cannot resolve correctly.
* Use in conjunction with {@link ActionBase.setFeedback} to update the layout's current items' settings.
packages/plugin/src/plugin/actions/action-base.ts:120
- The core regression is type-level, but the updated tests only exercise runtime forwarding with actions effectively typed as
JsonObject. A future reintroduction of a method-level generic would therefore pass them. Add a compile-time case using a concrete settings type that accepts its schema and marks an unrelated object with@ts-expect-error.
public setSettings(value: TSettings): Promise<void> {
packages/plugin/src/plugin/actions/action-base.ts:46
- This method no longer declares
U, so the@template Utag is stale; the trailingaction.Dis also erroneous. Document the class-level settings type through the return value instead.
* Gets the settings associated this action instance.
* @template U The type of settings associated with the action.D
* @returns Promise containing the action instance's settings.
packages/plugin/src/plugin/actions/singleton-action.ts:105
ActionBasehas nosendToPropertyInspectormethod; messages to the property inspector are sent bystreamDeck.ui. The current link points users to a nonexistent API.
This issue also appears on line 111 of the same file.
* Occurs when a message was sent to the plugin _from_ the property inspector. The plugin can also send messages _to_ the property inspector using {@link ActionBase.sendToPropertyInspector}.
packages/plugin/src/plugin/actions/service.ts:177
ActionBasedoes not definesetTitle; the operation belongs to the concrete key and dial action classes. Link those actual API members instead.
* Occurs when the user updates an action's title settings in the Stream Deck application. See also {@link ActionBase.setTitle}.
packages/plugin/src/plugin/actions/dial.ts:41
setFeedbackLayoutis declared onDialAction, notActionBase, so this link now targets a nonexistent member.
This issue also appears on line 58 of the same file.
* to users, and can be assigned in the manifest, or dynamically via {@link ActionBase.setFeedbackLayout}.
packages/plugin/src/plugin/actions/tests/action.test.ts:336
- This assertion invokes
showAlerton theKeyActiondeclared above;ActionBasehas no such method. The updated documentation link therefore points to a nonexistent member.
* Asserts {@link ActionBase.showAlert} forwards the command to the {@link connection}.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (9)
Previously missed (8) — in code that hasn't changed since the last review.
packages/plugin/src/plugin/actions/action-base.ts:45
- This method no longer declares
U, so the@template Udocumentation is stale and describes the loophole this refactor removes. Document the action-bound settings type instead.
* Gets the settings associated this action instance.
* @template U The type of settings associated with the action.D
packages/plugin/src/plugin/actions/action-base.ts:48
- The core change is a compile-time contract, but the updated tests only exercise runtime behavior and the package build excludes
__tests__. Add a type-checked fixture that verifies unrelated values and explicit return-type overrides are rejected, while the declared settings type is accepted.
public async getSettings(): Promise<TSettings> {
.changeset/free-oranges-lose.md:5
NeoInfobarActionalso lost its default settings type and is publicly exported, so this breaking-change note omits one affected API.
`Action`, `KeyAction`, and `DialAction` types now require their setting type be defined.
packages/plugin/src/plugin/actions/singleton-action.ts:105
ActionBasehas nosendToPropertyInspectormethod; that API belongs tostreamDeck.ui, so this new link cannot resolve to the documented operation.
* Occurs when a message was sent to the plugin _from_ the property inspector. The plugin can also send messages _to_ the property inspector using {@link ActionBase.sendToPropertyInspector}.
packages/plugin/src/plugin/actions/singleton-action.ts:111
ActionBasedoes not definesetTitle; only concrete key and dial actions do. Remove this unresolved link from the public event documentation.
* Occurs when the user updates an action's title settings in the Stream Deck application. See also {@link ActionBase.setTitle}.
packages/plugin/src/plugin/actions/service.ts:177
ActionBasedoes not definesetTitle, so this link points to a nonexistent member. Keep the event description without that invalid cross-reference.
* Occurs when the user updates an action's title settings in the Stream Deck application. See also {@link ActionBase.setTitle}.
packages/plugin/src/plugin/actions/dial.ts:41
setFeedbackLayoutis declared onDialAction, notActionBase; the new link targets a nonexistent base-class member.
This issue also appears on line 58 of the same file.
* to users, and can be assigned in the manifest, or dynamically via {@link ActionBase.setFeedbackLayout}.
packages/plugin/src/plugin/actions/tests/action.test.ts:336
- This test invokes
showAlerton aKeyAction, whileActionBasehas no such method. The updated link therefore points to a nonexistent member.
* Asserts {@link ActionBase.showAlert} forwards the command to the {@link connection}.
packages/plugin/src/plugin/actions/dial.ts:58
setFeedbackis declared onDialAction, notActionBase; update the link so generated documentation resolves to the actual method.
* Use in conjunction with {@link ActionBase.setFeedback} to update the layout's current items' settings.
| expect(listener).toHaveBeenCalledTimes(1); | ||
| expect(listener).toHaveBeenCalledWith<[DialRotateEvent<Settings>]>({ | ||
| action: actionStore.getActionById(ev.context) as DialAction, | ||
| action: actionStore.getActionById<JsonObject>(ev.context) as DialAction<Settings>, |
getSettingsandsetSettingsto always use the type defined on the action instance.Action<T>to represent a union of possible action types, e.g. dial, key, Neo infobar, etc.ActionBase<T>to replace previousAction<T>(remove from export).Relates to: #133