Skip to content

Commit 9823fc7

Browse files
VarixoVarixo
committed
feat: new merging logic for events in jsxSplit
1 parent beefac1 commit 9823fc7

File tree

6 files changed

+436
-257
lines changed

6 files changed

+436
-257
lines changed

‎packages/qwik/src/core/shared/jsx/jsx-internal.ts‎

Lines changed: 38 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { untrack } from '../../use/use-core';
22
import type { OnRenderFn } from '../component.public';
33
import { createQRL } from '../qrl/qrl-class';
44
import type { QRLInternal } from '../qrl/qrl-class';
5-
import { jsxEventToHtmlAttribute, isHtmlAttributeAnEventName } from '../utils/event-names';
5+
import { jsxEventToHtmlAttribute } from '../utils/event-names';
66
import { logOnceWarn } from '../utils/log';
77
import type { OnRenderProp, QSlot, QSlotS, QScopedStyle, ELEMENT_ID } from '../utils/markers';
88
import { qDev } from '../utils/qdev';
@@ -74,6 +74,8 @@ export const _jsxSplit = <T extends string | FunctionComponent<any>>(
7474
let toSort = false;
7575
let constPropsCopied = false;
7676
let varPropsCopied = false;
77+
let bindValueSignal: any = null;
78+
let bindCheckedSignal: any = null;
7779

7880
// Apply transformations for native HTML elements only
7981
if (typeof type === 'string') {
@@ -86,7 +88,7 @@ export const _jsxSplit = <T extends string | FunctionComponent<any>>(
8688
constProps = { ...constProps };
8789
constPropsCopied = true;
8890
}
89-
mergeHandlers(constProps, attr, constProps[k] as any);
91+
constProps[attr] = constProps[k];
9092
delete constProps[k];
9193
}
9294
}
@@ -99,114 +101,60 @@ export const _jsxSplit = <T extends string | FunctionComponent<any>>(
99101
varProps = { ...varProps };
100102
varPropsCopied = true;
101103
}
102-
// Check if the converted attribute already exists in constProps
103-
if (constProps && _hasOwnProperty.call(constProps, attr)) {
104-
// Merge into constProps since it already has this event
105-
if (!constPropsCopied) {
106-
constProps = { ...constProps };
107-
constPropsCopied = true;
108-
}
109-
mergeHandlers(constProps, attr, varProps[k] as any);
110-
} else {
111-
// Add to varProps
112-
toSort = mergeHandlers(varProps, attr, varProps[k] as any) || toSort;
113-
}
104+
// Transform event name in place
105+
varProps[attr] = varProps[k];
114106
delete varProps[k];
115-
} else if (isHtmlAttributeAnEventName(k)) {
116-
// Already in on:* format, check if it exists in constProps
117-
if (constProps && _hasOwnProperty.call(constProps, k)) {
118-
if (!varPropsCopied) {
119-
varProps = { ...varProps };
120-
varPropsCopied = true;
121-
}
122-
if (!constPropsCopied) {
123-
constProps = { ...constProps };
124-
constPropsCopied = true;
125-
}
126-
// Merge into constProps
127-
mergeHandlers(constProps, k, varProps[k] as any);
128-
delete varProps[k];
129-
}
107+
toSort = true;
108+
} else if (k === BIND_CHECKED) {
109+
// Set flag, will process after walk
110+
bindCheckedSignal = varProps[k];
111+
} else if (k === BIND_VALUE) {
112+
// Set flag, will process after walk
113+
bindValueSignal = varProps[k];
130114
}
131115
}
132-
}
133116

134-
// Handle bind:*
135-
if (varProps) {
136-
if (_hasOwnProperty.call(varProps, BIND_CHECKED)) {
117+
// Handle bind:* - only in varProps (constProps are transformed by Rust)
118+
if (bindCheckedSignal || bindValueSignal) {
137119
if (!varPropsCopied) {
138120
varProps = { ...varProps };
139121
varPropsCopied = true;
140122
}
141-
const value = varProps[BIND_CHECKED];
142-
delete varProps[BIND_CHECKED];
143-
if (value) {
144-
varProps.checked = value;
145-
const handler = createQRL(null, '_chk', _chk, null, null, [value]);
146-
// Check if on:input already exists in constProps
123+
124+
if (bindCheckedSignal) {
125+
delete varProps[BIND_CHECKED];
126+
varProps.checked = bindCheckedSignal;
127+
const handler = createQRL(null, '_chk', _chk, null, null, [bindCheckedSignal]);
128+
129+
// Move on:input from constProps if it exists
147130
if (constProps && _hasOwnProperty.call(constProps, 'on:input')) {
148131
if (!constPropsCopied) {
149132
constProps = { ...constProps };
150133
constPropsCopied = true;
151134
}
152-
mergeHandlers(constProps, 'on:input', handler);
153-
} else {
154-
toSort = mergeHandlers(varProps, 'on:input', handler) || toSort;
135+
const existingHandler = constProps['on:input'];
136+
delete constProps['on:input'];
137+
toSort = mergeHandlers(varProps, 'on:input', existingHandler as any) || toSort;
155138
}
156-
}
157-
} else if (_hasOwnProperty.call(varProps, BIND_VALUE)) {
158-
if (!varPropsCopied) {
159-
varProps = { ...varProps };
160-
varPropsCopied = true;
161-
}
162-
const value = varProps[BIND_VALUE];
163-
delete varProps[BIND_VALUE];
164-
if (value) {
165-
varProps.value = value;
166-
const handler = createQRL(null, '_val', _val, null, null, [value]);
167-
// Check if on:input already exists in constProps
139+
140+
toSort = mergeHandlers(varProps, 'on:input', handler) || toSort;
141+
} else if (bindValueSignal) {
142+
delete varProps[BIND_VALUE];
143+
varProps.value = bindValueSignal;
144+
const handler = createQRL(null, '_val', _val, null, null, [bindValueSignal]);
145+
146+
// Move on:input from constProps if it exists
168147
if (constProps && _hasOwnProperty.call(constProps, 'on:input')) {
169148
if (!constPropsCopied) {
170149
constProps = { ...constProps };
171150
constPropsCopied = true;
172151
}
173-
mergeHandlers(constProps, 'on:input', handler);
174-
} else {
175-
toSort = mergeHandlers(varProps, 'on:input', handler) || toSort;
152+
const existingHandler = constProps['on:input'];
153+
delete constProps['on:input'];
154+
toSort = mergeHandlers(varProps, 'on:input', existingHandler as any) || toSort;
176155
}
177-
}
178-
}
179-
}
180-
if (constProps) {
181-
if (_hasOwnProperty.call(constProps, BIND_CHECKED)) {
182-
if (!constPropsCopied) {
183-
constProps = { ...constProps };
184-
constPropsCopied = true;
185-
}
186-
const value = constProps[BIND_CHECKED];
187-
delete constProps[BIND_CHECKED];
188-
if (value) {
189-
constProps.checked = value;
190-
mergeHandlers(
191-
constProps,
192-
'on:input',
193-
createQRL(null, '_chk', _chk, null, null, [value])
194-
);
195-
}
196-
} else if (_hasOwnProperty.call(constProps, BIND_VALUE)) {
197-
if (!constPropsCopied) {
198-
constProps = { ...constProps };
199-
constPropsCopied = true;
200-
}
201-
const value = constProps[BIND_VALUE];
202-
delete constProps[BIND_VALUE];
203-
if (value) {
204-
constProps.value = value;
205-
mergeHandlers(
206-
constProps,
207-
'on:input',
208-
createQRL(null, '_val', _val, null, null, [value])
209-
);
156+
157+
toSort = mergeHandlers(varProps, 'on:input', handler) || toSort;
210158
}
211159
}
212160
}

‎packages/qwik/src/core/shared/jsx/jsx-split.unit.ts‎

Lines changed: 46 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -40,71 +40,43 @@ describe('_jsxSplit', () => {
4040
});
4141
});
4242

43-
describe('event merging between constProps and varProps', () => {
44-
it('should merge handlers when constProps has on:input and varProps has onInput$', () => {
45-
const handler1 = (() => {}) as any as QRL;
46-
const handler2 = (() => {}) as any as QRL;
43+
describe('event handling between constProps and varProps', () => {
44+
it('should let constProps overwrite varProps when both have same event', () => {
45+
const constHandler = (() => {}) as any as QRL;
46+
const varHandler = (() => {}) as any as QRL;
4747

48-
const node = _jsxSplit('input', { onInput$: handler2 }, { 'on:input': handler1 }, null, 0);
48+
const node = _jsxSplit(
49+
'input',
50+
{ onInput$: varHandler },
51+
{ 'on:input': constHandler },
52+
null,
53+
0
54+
);
4955

50-
// varProps should not have onInput$ anymore
56+
// varProps should be transformed but then removed during deduplication
5157
expect(node.varProps['onInput$']).toBeUndefined();
58+
expect(node.varProps['on:input']).toBeUndefined();
5259

53-
// constProps should have the merged handler
54-
const merged = node.constProps?.['on:input'];
55-
expect(merged).toBeDefined();
56-
57-
// Should be an array with both handlers
58-
expect(Array.isArray(merged)).toBe(true);
59-
if (Array.isArray(merged)) {
60-
expect(merged).toHaveLength(2);
61-
expect(merged[0]).toBe(handler1);
62-
expect(merged[1]).toBe(handler2);
63-
}
60+
// constProps keeps its handler (no merging)
61+
expect(node.constProps?.['on:input']).toBe(constHandler);
6462
});
6563

66-
it('should merge handlers when constProps has onInput$ and varProps has on:input', () => {
67-
const handler1 = (() => {}) as any as QRL;
68-
const handler2 = (() => {}) as any as QRL;
64+
it('should let constProps overwrite varProps after transformation', () => {
65+
const constHandler = (() => {}) as any as QRL;
66+
const varHandler = (() => {}) as any as QRL;
6967

70-
const node = _jsxSplit('input', { 'on:input': handler2 }, { onInput$: handler1 }, null, 0);
71-
72-
// constProps should have onInput$ converted to on:input
73-
expect(node.constProps?.['onInput$']).toBeUndefined();
74-
expect(node.constProps?.['on:input']).toBeDefined();
75-
76-
const merged = node.constProps?.['on:input'];
77-
78-
// Should be an array with both handlers
79-
expect(Array.isArray(merged)).toBe(true);
80-
if (Array.isArray(merged)) {
81-
expect(merged).toHaveLength(2);
82-
expect(merged[0]).toBe(handler1);
83-
expect(merged[1]).toBe(handler2);
84-
}
85-
});
86-
87-
it('should merge handlers when both have onInput$ (different sources)', () => {
88-
const handler1 = (() => {}) as any as QRL;
89-
const handler2 = (() => {}) as any as QRL;
90-
91-
const node = _jsxSplit('input', { onInput$: handler2 }, { onInput$: handler1 }, null, 0);
92-
93-
// varProps should not have onInput$ anymore
94-
expect(node.varProps['onInput$']).toBeUndefined();
68+
const node = _jsxSplit(
69+
'input',
70+
{ 'on:input': varHandler },
71+
{ onInput$: constHandler },
72+
null,
73+
0
74+
);
9575

96-
// constProps should have onInput$ converted to on:input with merged handlers
76+
// constProps transformed and constProps wins
9777
expect(node.constProps?.['onInput$']).toBeUndefined();
98-
const merged = node.constProps?.['on:input'];
99-
expect(merged).toBeDefined();
100-
101-
// Should be an array with both handlers
102-
expect(Array.isArray(merged)).toBe(true);
103-
if (Array.isArray(merged)) {
104-
expect(merged).toHaveLength(2);
105-
expect(merged[0]).toBe(handler1);
106-
expect(merged[1]).toBe(handler2);
107-
}
78+
expect(node.constProps?.['on:input']).toBe(constHandler);
79+
expect(node.varProps['on:input']).toBeUndefined();
10880
});
10981

11082
it('should handle multiple different events in both props', () => {
@@ -127,46 +99,28 @@ describe('_jsxSplit', () => {
12799
0
128100
);
129101

130-
// Check click handlers merged
131-
const clickMerged = node.constProps?.['on:click'];
132-
expect(Array.isArray(clickMerged)).toBe(true);
133-
if (Array.isArray(clickMerged)) {
134-
expect(clickMerged).toHaveLength(2);
135-
expect(clickMerged[0]).toBe(clickHandler1);
136-
expect(clickMerged[1]).toBe(clickHandler2);
137-
}
138-
139-
// Check input handlers merged
140-
const inputMerged = node.constProps?.['on:input'];
141-
expect(Array.isArray(inputMerged)).toBe(true);
142-
if (Array.isArray(inputMerged)) {
143-
expect(inputMerged).toHaveLength(2);
144-
expect(inputMerged[0]).toBe(inputHandler1);
145-
expect(inputMerged[1]).toBe(inputHandler2);
146-
}
102+
// constProps wins for both (no merging)
103+
expect(node.constProps?.['on:click']).toBe(clickHandler1);
104+
expect(node.constProps?.['on:input']).toBe(inputHandler1);
105+
expect(node.varProps['on:click']).toBeUndefined();
106+
expect(node.varProps['on:input']).toBeUndefined();
147107
});
148108

149-
it('should handle array of handlers in constProps', () => {
150-
const handler1 = (() => {}) as any as QRL;
151-
const handler2 = (() => {}) as any as QRL;
152-
const handler3 = (() => {}) as any as QRL;
109+
it('should keep events separate when no overlap', () => {
110+
const clickHandler = (() => {}) as any as QRL;
111+
const inputHandler = (() => {}) as any as QRL;
153112

154113
const node = _jsxSplit(
155114
'input',
156-
{ onInput$: handler3 },
157-
{ 'on:input': [handler1, handler2] as any },
115+
{ onClick$: clickHandler },
116+
{ onInput$: inputHandler },
158117
null,
159118
0
160119
);
161120

162-
const merged = node.constProps?.['on:input'];
163-
expect(Array.isArray(merged)).toBe(true);
164-
if (Array.isArray(merged)) {
165-
expect(merged).toHaveLength(3);
166-
expect(merged[0]).toBe(handler1);
167-
expect(merged[1]).toBe(handler2);
168-
expect(merged[2]).toBe(handler3);
169-
}
121+
// Both should be kept in their respective locations
122+
expect(node.varProps['on:click']).toBe(clickHandler);
123+
expect(node.constProps?.['on:input']).toBe(inputHandler);
170124
});
171125
});
172126

@@ -198,7 +152,7 @@ describe('_jsxSplit', () => {
198152
}
199153
});
200154

201-
it('should merge bind:value handler with existing on:input in constProps', () => {
155+
it('should move on:input from constProps to varProps when bind:value is in varProps', () => {
202156
const handler = (() => {}) as any as QRL;
203157
const signal = { value: 'test' };
204158

@@ -217,13 +171,14 @@ describe('_jsxSplit', () => {
217171
expect(node.varProps['bind:value']).toBeUndefined();
218172
expect(node.varProps.value).toBe(signal);
219173

220-
// When constProps has on:input, bind handler should merge into constProps
221-
const merged = node.constProps?.['on:input'];
174+
// on:input should be moved from constProps to varProps and merged with bind handler
175+
expect(node.constProps?.['on:input']).toBeUndefined();
176+
const merged = node.varProps['on:input'];
222177
expect(merged).toBeDefined();
223178
expect(Array.isArray(merged)).toBe(true);
224179
if (Array.isArray(merged)) {
225180
expect(merged).toHaveLength(2);
226-
expect(merged[0]).toBe(handler); // Original handler from constProps
181+
expect(merged[0]).toBe(handler); // Original handler from constProps (moved)
227182
// merged[1] should be the bind handler
228183
}
229184
});

0 commit comments

Comments
 (0)