feat: type in-page channel functions from protocol - #314
Conversation
|
@posva is attempting to deploy a commit to the NuxtLabs Team on Vercel. A member of the Team first needs to authorize it. |
|
I still want to improve the functions type: I think ideally it should be an object so that all functions are defined and it emits a type error if any is missing |
| const registry = createLocalFunctionRegistry(codec) | ||
| for (const definition of options.functions ?? []) | ||
| registry.register(definition) | ||
| for (const [fnName, definition] of Object.entries(options.functions ?? {})) |
There was a problem hiding this comment.
Here we could use for in, it's a bit faster but goes through inherited properties, which in the case of functions, should be none unless someone pollutes the object prototype. I kept it this way because it shouldn't change much in the end
| const registry = createLocalFunctionRegistry(codec) | ||
| for (const definition of options.functions ?? []) | ||
| registry.register(definition) | ||
| for (const [fnName, definition] of Object.entries(options.functions ?? {})) |
There was a problem hiding this comment.
same as in page-script.ts
| export interface CreatePageScriptChannelOptions extends InPageChannelCommonOptions { | ||
| export interface CreatePageScriptChannelOptions<Protocol extends InPageChannelProtocol = InPageChannelProtocol> extends InPageChannelCommonOptions { | ||
| /** Implementations of the protocol's page-script functions. */ | ||
| functions: CreatePageScriptChannelOptionsFunctions<Protocol> |
There was a problem hiding this comment.
I made the functions intentionally required here because you usually must define them and is more type safe
There was a problem hiding this comment.
Pull request overview
This PR refactors devframe/in-page-channel to type each endpoint’s registered function handlers directly from the shared InPageChannelProtocol, switching function registration from an array of named definitions to an object keyed by function name. This improves type inference and enforces that each endpoint implements the functions declared for its side of the protocol.
Changes:
- Updated in-page channel endpoint options to require
functionsas an object keyed by protocol function names, with handlers contextually typed from the protocol. - Migrated the a11y devframe’s page script + panel channel wiring to the new
functionsobject shape. - Added/updated Vitest type tests (
*.test-d.ts) and runtime tests to validate the new typing and registration behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/a11y/src/spa/lib/channel.ts | Updates panel endpoint channel connection to provide required functions: {} under the new options shape. |
| plugins/a11y/src/inject/index.ts | Migrates page script endpoint function registration from defineChannelFunction[] to an object keyed by protocol function names. |
| packages/devframe/vitest.config.ts | Adds a per-package Vitest config enabling typecheck runs for devframe. |
| packages/devframe/src/in-page-channel/types.ts | Introduces protocol-keyed typing for endpoint functions options and makes functions required per endpoint. |
| packages/devframe/src/in-page-channel/types.test-d.ts | Adds type-level tests ensuring handler inference and correct call-site typing for both endpoints. |
| packages/devframe/src/in-page-channel/panel.ts | Switches function registration to iterate object entries and register definitions by key name. |
| packages/devframe/src/in-page-channel/page-script.ts | Switches function registration to iterate object entries and register definitions by key name. |
| packages/devframe/src/in-page-channel/in-page-channel.test.ts | Updates runtime tests to the new functions object shape and adds typed defaults for reuse. |
| docs/content/1.guide/12.in-page-channel.md | Updates documentation examples to the new functions object keyed API. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const registry = createLocalFunctionRegistry(codec) | ||
| for (const definition of options.functions ?? []) | ||
| registry.register(definition) | ||
| for (const [fnName, definition] of Object.entries(options.functions ?? {})) | ||
| registry.register({ ...definition, name: fnName }) |
| const registry = createLocalFunctionRegistry(codec) | ||
| for (const definition of options.functions ?? []) | ||
| registry.register(definition) | ||
| for (const [fnName, definition] of Object.entries(options.functions ?? {})) | ||
| registry.register({ ...definition, name: fnName }) |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/devframe/src/in-page-channel/panel.ts:66
options.functionsis an object keyed by function name, but anynamefield present on the provided definition is silently ignored/overwritten by the key. This is a footgun if someone reusesdefineChannelFunction({ name: ... })(or otherwise includesname) and the key/name diverge — the function will be registered under the key, and remote calls to thedefinition.namewill fail at runtime. Consider validating key/name consistency before registering to fail fast.
for (const [fnName, definition] of Object.entries(options.functions ?? {}))
registry.register({ ...definition, name: fnName })
packages/devframe/src/in-page-channel/page-script.ts:67
options.functionsis keyed by function name, but anynamefield on a provided definition is silently ignored/overwritten by the key. If a caller passes a named definition (e.g. fromdefineChannelFunction) with a mismatched key, the handler will be registered under the key and calls to the declared name will fail at runtime. Validating the key/name match here would make these mistakes fail fast.
const registry = createLocalFunctionRegistry(codec)
for (const [fnName, definition] of Object.entries(options.functions ?? {}))
registry.register({ ...definition, name: fnName })
| const channel = connectPanelChannel<A11yChannelProtocol>({ | ||
| name: A11Y_CHANNEL, | ||
| functions: {}, | ||
| }) |
| export interface CreatePageScriptChannelOptions<Protocol extends InPageChannelProtocol = InPageChannelProtocol> extends InPageChannelCommonOptions { | ||
| /** Implementations of the protocol's page-script functions. */ | ||
| functions: CreatePageScriptChannelOptionsFunctions<Protocol> |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/devframe/src/in-page-channel/types.ts:109
- The
returnsfield is documented as a Standard Schema that validates the resolved return value, but the current in-page channel implementation never readsdefinition.returns(onlydefinition.argsis validated). This comment is misleading unless return-value validation is implemented.
jsonSerializable?: boolean
handler: ProtocolHandler<F>
}
/**
docs/content/1.guide/12.in-page-channel.md:57
- This section says the in-page channel uses Standard-Schema
args/returns, but the current implementation only performs runtime validation forargs(there is no return-value validation). The wording should avoid implying return schemas are enforced.
Functions use the same authoring metadata as `defineRpcFunction` (`type`, Standard-Schema `args`/`returns`, `jsonSerializable`, `handler`), narrowed to the browser. The required `functions` object's keys are the function names, and it implements every function on that endpoint's protocol side. Each handler is contextually typed from its key and the corresponding function in the protocol. `defineChannelFunction` retains the named definition shape for lower-level authoring. Define each side's functions in that side's source files; the shared protocol file carries only types.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/devframe/src/in-page-channel/in-page-channel.test.ts:207
- The comment says panel B has no local functions "in its protocol", but the generic type used here is the broad
InPageChannelProtocol(not a protocol with an explicit emptypanelmap). This can read as if the protocol type itself is empty rather than just the local handler map being empty. Consider rewording to describe the implementation intent (no local handlers) to avoid confusion for future readers.
// Panel B deliberately has no local functions in its protocol.
const panelB = connectPanelChannel<InPageChannelProtocol>({
| * results flow back through the shared state. | ||
| */ | ||
| export interface A11yChannelProtocol extends InPageChannelProtocol { | ||
| export interface A11yChannelProtocol { |
There was a problem hiding this comment.
The extends makes it get panel: Record<string, ...> which allows anything
No description provided.