-
-
Notifications
You must be signed in to change notification settings - Fork 825
Move ReactiveController and Host to core #6548
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
base: main
Are you sure you want to change the base?
Move ReactiveController and Host to core #6548
Conversation
…community as a pattern for creating & sharing controllers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request moves the ReactiveController interface and ReactiveControllerHost class from internal test files into the public Stencil Core API. This enables developers to use the reactive controller pattern for component composition, similar to patterns found in frameworks like Lit.
Changes:
- Moved
ReactiveControllerandReactiveControllerHostfrom test-local implementations tosrc/runtime/reactive-controller.tsas public exports - Updated all test files to import from
@stencil/coreinstead of local./reactive-controller-host.jsfiles - Added proper TypeScript type declarations in
stencil-public-runtime.ts
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/runtime/reactive-controller.ts | Implements the ReactiveController interface and ReactiveControllerHost class with lifecycle hooks |
| src/runtime/index.ts | Exports ReactiveController and ReactiveControllerHost from the runtime module |
| src/declarations/stencil-public-runtime.ts | Adds public TypeScript type declarations for the reactive controller API |
| src/internal/stencil-core/index.d.ts | Exports the new public types and classes in the main API surface |
| src/compiler/transformers/update-stencil-core-import.ts | Adds ReactiveController and ReactiveControllerHost to KEEP_IMPORTS list |
| src/hydrate/platform/index.ts | Re-exports the new API for server-side rendering support |
| test/wdio/ts-target/extends-composition-scaling/reactive-controller-host.ts | Deleted - local implementation replaced by public API |
| test/wdio/ts-target/extends-via-host/mouse-controller.ts | Updated import to use @stencil/core |
| test/wdio/ts-target/extends-via-host/cmp.tsx | Updated import to use @stencil/core |
| test/wdio/ts-target/extends-composition-scaling/validation-controller.ts | Updated import to use @stencil/core |
| test/wdio/ts-target/extends-composition-scaling/text-input-cmp.tsx | Updated import to use @stencil/core |
| test/wdio/ts-target/extends-composition-scaling/radio-group-cmp.tsx | Updated import to use @stencil/core |
| test/wdio/ts-target/extends-composition-scaling/focus-controller.ts | Updated import to use @stencil/core |
| test/wdio/ts-target/extends-composition-scaling/checkbox-group-cmp.tsx | Updated import to use @stencil/core |
Comments suppressed due to low confidence (1)
src/runtime/reactive-controller.ts:62
- Extra blank line at end of file should be removed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| componentWillLoad(): Promise<void> | void; | ||
| componentDidLoad(): void; | ||
| componentWillRender(): Promise<void> | void; | ||
| componentDidRender(): void; | ||
| componentWillUpdate(): Promise<void> | void; |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type declaration shows componentWillLoad() returns Promise<void> | void, but the actual implementation in src/runtime/reactive-controller.ts (line 38-40) doesn't return anything and only calls controller hooks. The declaration should match the implementation's return type or the implementation should properly handle async controller hooks.
| componentWillLoad(): Promise<void> | void; | |
| componentDidLoad(): void; | |
| componentWillRender(): Promise<void> | void; | |
| componentDidRender(): void; | |
| componentWillUpdate(): Promise<void> | void; | |
| componentWillLoad(): void; | |
| componentDidLoad(): void; | |
| componentWillRender(): void; | |
| componentDidRender(): void; | |
| componentWillUpdate(): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OS-paulvisciano can you please check this out ☝🏻
gnbm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OS-paulvisciano Two checks are failing:
- CI / Lint and Format / Check (pull_request)
CI / Lint and Format / Check (pull_request)Failing after 46s
Required - CI / WebdriverIO Tests / Run WebdriverIO Component Tests (CHROME) (pull_request)
CI / WebdriverIO Tests / Run WebdriverIO Component Tests (CHROME) (pull_request)
And ideally it would be good practice to use signed commits
| 'jsx', | ||
| 'jsxs', | ||
| 'jsxDEV', | ||
| 'ReactiveController', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this list are imports that actively have a runtime component - ReactiveController is an interface only(?) so shouldn't be here
| postUpdateComponent, | ||
| proxyComponent, | ||
| proxyCustomElement, | ||
| ReactiveController, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again - I don't think an interface should be in this list
Exposing ReactiveController and Host publicly to make it easier to implement the pattern and also provide a common interface community members can use to create and share Controllers
What is the current behavior?
The classes were internal to the tests
What is the new behavior?
Exposed as part of the public API
Documentation
For more info on Reactive Controllers
https://stenciljs.com/docs/extends
Does this introduce a breaking change?
Testing
Existing tests using the Reactive controller pattern now import the implementation from core and pass.