Skip to content

chore(): 修复如果brick设置了原生属性报错问题 #4854

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.

Sign up for GitHub

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

Open
WHChen-Alex wants to merge 1 commit into
base: v3
Choose a base branch
from
Open

chore(): 修复如果brick设置了原生属性报错问题 #4854

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 26 additions & 15 deletions packages/react-element/src/wrapBrick.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,26 @@ type MapEvents<E, M> = {
[key in keyof M]?: M[key] extends keyof E ? (e: E[M[key]]) => void : never;
};

export function wrapLocalBrick<T extends HTMLElement, P, E, M extends object>(
export function wrapLocalBrick<
T extends Partial<HTMLElement>,
P,
E,
M extends object,
>(
Comment on lines +28 to +33
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The new generic constraint T extends Partial<HTMLElement> makes almost any object type assignable (since all HTMLElement members are optional), which means ref/event handler generics can now be something that is not actually a DOM element. If the goal is only to relax structural compatibility while still guaranteeing a real element instance, consider constraining T to a DOM base type (e.g. Element/HTMLElement) and introducing a separate type parameter for any “looser” view of the instance, rather than weakening the element constraint to Partial<HTMLElement>.

Copilot uses AI. Check for mistakes.
brick: Constructable<T> | string,
eventsMapping: M
): WrappedBrickWithEventsMap<T, P, E, M>;

export function wrapLocalBrick<T extends HTMLElement, P>(
export function wrapLocalBrick<T extends Partial<HTMLElement>, P>(
brick: Constructable<T> | string
): WrappedBrick<T, P>;
Comment on lines +38 to 40
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

This change is intended to fix a typing error scenario, but there’s no regression coverage that would fail before and pass after (e.g. a compile-time usage that previously violated T extends HTMLElement). Consider adding a minimal type-level regression (even just a non-runtime "compiles" usage in the existing Jest TS test file) that exercises a brick instance type including native attributes/properties that previously triggered the error, so the issue doesn’t regress silently.

Copilot uses AI. Check for mistakes.

export function wrapLocalBrick<T extends HTMLElement, P, _E, M extends object>(
brick: Constructable<T> | string,
eventsMapping?: M
) {
export function wrapLocalBrick<
T extends Partial<HTMLElement>,
P,
_E,
M extends object,
>(brick: Constructable<T> | string, eventsMapping?: M) {
// istanbul ignore next
if (process.env.NODE_ENV === "development") {
if (typeof brick === "string" && !customElements.get(brick)) {
Expand All @@ -62,19 +69,23 @@ export function wrapLocalBrick<T extends HTMLElement, P, _E, M extends object>(
);
}

export function wrapBrick<T extends HTMLElement, P, E, M extends object>(
BrickName: string,
eventsMapping: M
): WrappedBrickWithEventsMap<T, P, E, M>;
export function wrapBrick<
T extends Partial<HTMLElement>,
P,
E,
M extends object,
>(BrickName: string, eventsMapping: M): WrappedBrickWithEventsMap<T, P, E, M>;
Comment on lines +72 to +77
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

Same concern for wrapBrick: switching T to Partial<HTMLElement> widens the public API so wrapBrick<Foo, P>(...) can compile even when Foo is not a DOM element type, which can make ref typing misleading. If you need to support bricks whose instance types aren’t structurally compatible with HTMLElement, consider a safer bound (e.g. T extends Element) and/or a separate generic for the loose instance view used by callers.

Copilot uses AI. Check for mistakes.

export function wrapBrick<T extends HTMLElement, P>(
export function wrapBrick<T extends Partial<HTMLElement>, P>(
BrickName: string
): WrappedBrick<T, P>;

export function wrapBrick<T extends HTMLElement, P, _E, M extends object>(
BrickName: string,
eventsMapping?: M
) {
export function wrapBrick<
T extends Partial<HTMLElement>,
P,
_E,
M extends object,
>(BrickName: string, eventsMapping?: M) {
return forwardRef<T, HTMLAttributes<T> & PropsWithChildren<P>>(
function BrickReactWrapper({ children, ...props }, ref: Ref<T>) {
const properties = getMappedProperties(
Expand Down
Loading