Skip to content

Commit

Permalink
fix: prevent infinity loop by inserting the same projection before it…
Browse files Browse the repository at this point in the history 
…self
  • Loading branch information
@Varixo
Varixo committed Feb 21, 2025
1 parent 7ef293a commit d82f44a
Show file tree
Hide file tree
Showing 13 changed files with 165 additions and 43 deletions.
5 changes: 5 additions & 0 deletions .changeset/five-shoes-deny.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@qwik.dev/core': patch
---

fix: prevent infinity loop by inserting the same projection before itself
3 changes: 2 additions & 1 deletion packages/qwik/src/core/client/dom-container.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
Q_PROPS_SEPARATOR,
USE_ON_LOCAL_SEQ_IDX,
getQFuncs,
QLocaleAttr,
} from '../shared/utils/markers';
import { isPromise } from '../shared/utils/promises';
import { isSlotProp } from '../shared/utils/prop';
Expand Down Expand Up @@ -141,7 +142,7 @@ export class DomContainer extends _SharedContainer implements IClientContainer {
() => this.scheduleRender(),
() => vnode_applyJournal(this.$journal$),
{},
element.getAttribute('q:locale')!
element.getAttribute(QLocaleAttr)!
);
this.qContainer = element.getAttribute(QContainerAttr)!;
if (!this.qContainer) {
Expand Down
23 changes: 19 additions & 4 deletions packages/qwik/src/core/client/vnode-diff.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ import {
vnode_getType,
vnode_insertBefore,
vnode_isElementVNode,
vnode_isProjection,
vnode_isTextVNode,
vnode_isVNode,
vnode_isVirtualVNode,
Expand All @@ -94,7 +95,7 @@ import { getNewElementNamespaceData } from './vnode-namespace';
import { WrappedSignal, EffectProperty, isSignal, SubscriptionData } from '../signal/signal';
import type { Signal } from '../signal/signal.public';
import { executeComponent } from '../shared/component-execution';
import { isParentSlotProp, isSlotProp } from '../shared/utils/prop';
import { isSlotProp } from '../shared/utils/prop';
import { escapeHTML } from '../shared/utils/character-escaping';
import { clearAllEffects } from '../signal/signal-cleanup';
import { serializeAttribute } from '../shared/utils/styles';
Expand Down Expand Up @@ -507,6 +508,21 @@ export const vnode_diff = (
// All is good.
// console.log(' NOOP', String(vCurrent));
} else {
const parent = vnode_getParent(vProjectedNode);
const isAlreadyProjected =
!!parent && !(vnode_isElementVNode(parent) && vnode_getElementName(parent) === QTemplate);
if (isAlreadyProjected && vParent !== parent) {
/**
* The node is already projected, but structure has been changed. In next steps we will
* insert the vProjectedNode at the end. However we will find existing projection elements
* (from already projected THE SAME projection as vProjectedNode!) during
* vnode_insertBefore. We need to remove vnode from the vnode tree to avoid referencing it
* to self and cause infinite loop. Don't remove it from DOM to avoid additional operations
* and flickering.
*/
vnode_remove(journal, parent, vProjectedNode, false);
}

// move from q:template to the target node
vnode_insertBefore(
journal,
Expand Down Expand Up @@ -1329,7 +1345,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) {
const attrs = vnode_getProps(vCursor);
for (let i = 0; i < attrs.length; i = i + 2) {
const key = attrs[i] as string;
if (!isParentSlotProp(key) && isSlotProp(key)) {
if (isSlotProp(key)) {
const value = attrs[i + 1];
if (value) {
attrs[i + 1] = null; // prevent infinite loop
Expand All @@ -1349,8 +1365,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) {
}
}

const isProjection =
type & VNodeFlags.Virtual && vnode_getProp(vCursor as VirtualVNode, QSlot, null) !== null;
const isProjection = vnode_isProjection(vCursor);
// Descend into children
if (!isProjection) {
// Only if it is not a projection
Expand Down
20 changes: 15 additions & 5 deletions packages/qwik/src/core/client/vnode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ import {
QScopedStyle,
QSlot,
QSlotParent,
QSlotRef,
QStyle,
QStylesAllSelector,
Q_PROPS_SEPARATOR,
Expand Down Expand Up @@ -333,6 +332,15 @@ export const vnode_isVirtualVNode = (vNode: VNode): vNode is VirtualVNode => {
return (flag & VNodeFlags.Virtual) === VNodeFlags.Virtual;
};

export const vnode_isProjection = (vNode: VNode): vNode is VirtualVNode => {
assertDefined(vNode, 'Missing vNode');
const flag = (vNode as VNode)[VNodeProps.flags];
return (
(flag & VNodeFlags.Virtual) === VNodeFlags.Virtual &&
vnode_getProp(vNode as VirtualVNode, QSlot, null) !== null
);
};

const ensureTextVNode = (vNode: VNode): TextVNode => {
assertTrue(vnode_isTextVNode(vNode), 'Expecting TextVNode was: ' + vnode_getNodeTypeName(vNode));
return vNode as TextVNode;
Expand Down Expand Up @@ -401,7 +409,7 @@ export const vnode_ensureElementInflated = (vnode: VNode) => {
/** Walks the VNode tree and materialize it using `vnode_getFirstChild`. */
export function vnode_walkVNode(
vNode: VNode,
callback?: (vNode: VNode, vParent: VNode | null) => void
callback?: (vNode: VNode, vParent: VNode | null) => boolean | void
): void {
let vCursor: VNode | null = vNode;
// Depth first traversal
Expand All @@ -411,7 +419,9 @@ export function vnode_walkVNode(
}
let vParent: VNode | null = null;
do {
callback?.(vCursor, vParent);
if (callback?.(vCursor, vParent)) {
return;
}
const vFirstChild = vnode_getFirstChild(vCursor);
if (vFirstChild) {
vCursor = vFirstChild;
Expand Down Expand Up @@ -1794,8 +1804,6 @@ function materializeFromVNodeData(
isDev && vnode_setAttr(null, vParent, ELEMENT_ID, id);
} else if (peek() === VNodeDataChar.PROPS) {
vnode_setAttr(null, vParent, ELEMENT_PROPS, consumeValue());
} else if (peek() === VNodeDataChar.SLOT_REF) {
vnode_setAttr(null, vParent, QSlotRef, consumeValue());
} else if (peek() === VNodeDataChar.KEY) {
vnode_setAttr(null, vParent, ELEMENT_KEY, consumeValue());
} else if (peek() === VNodeDataChar.SEQ) {
Expand All @@ -1807,6 +1815,8 @@ function materializeFromVNodeData(
container = getDomContainer(element);
}
setEffectBackRefFromVNodeData(vParent, consumeValue(), container);
} else if (peek() === VNodeDataChar.SLOT_PARENT) {
vnode_setProp(vParent, QSlotParent, consumeValue());
} else if (peek() === VNodeDataChar.CONTEXT) {
vnode_setAttr(null, vParent, QCtxAttr, consumeValue());
} else if (peek() === VNodeDataChar.OPEN) {
Expand Down
4 changes: 2 additions & 2 deletions packages/qwik/src/core/client/vnode.unit.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,10 @@ describe('vnode', () => {
});
it('should place attributes on Virtual', () => {
parent.innerHTML = ``;
document.qVNodeData.set(parent, '{?:sref_@:key_}');
document.qVNodeData.set(parent, '{?:sparent_@:key_}');
expect(vParent).toMatchVDOM(
<test>
<Fragment {...({ 'q:sref': ':sref_' } as any)} key=":key_" />
<Fragment {...({ 'q:sparent': ':sparent_' } as any)} key=":key_" />
</test>
);
});
Expand Down
18 changes: 1 addition & 17 deletions packages/qwik/src/core/shared/utils/markers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,12 @@ import { QContainerValue } from '../types';
/** State factory of the component. */
export const OnRenderProp = 'q:renderFn';

/** Component style host prefix */
export const ComponentStylesPrefixHost = '💎';

/** Component style content prefix */
export const ComponentStylesPrefixContent = '⚡️';

/** Prefix used to identify on listeners. */
export const EventPrefix = 'on:';

/** Attribute used to mark that an event listener is attached. */
export const EventAny = 'on:.';
/** `<some-element q:slot="...">` */
export const QSlot = 'q:slot';
export const QSlotParent = ':';
export const QSlotRef = 'q:sref';
export const QSlotParent = 'q:sparent';
export const QSlotS = 'q:s';
export const QStyle = 'q:style';
export const QStyleSelector = 'style[q\\:style]';
Expand All @@ -26,7 +17,6 @@ export const QStylesAllSelector = QStyleSelector + ',' + QStyleSSelector;
export const QScopedStyle = 'q:sstyle';
export const QCtxAttr = 'q:ctx';
export const QBackRefs = 'q:brefs';
export const QManifestHash = 'q:manifest-hash';
export const QFuncsPrefix = 'qFuncs_';

export const getQFuncs = (
Expand All @@ -49,7 +39,6 @@ export const QIgnore = 'q:ignore';
export const QIgnoreEnd = '/' + QIgnore;
export const QContainerAttr = 'q:container';
export const QContainerAttrEnd = '/' + QContainerAttr;
export const QShadowRoot = 'q:shadowroot';

export const QTemplate = 'q:template';

Expand All @@ -72,7 +61,6 @@ export const RenderEvent = 'qRender';
export const TaskEvent = 'qTask';

/** `<q:slot name="...">` */
export const QSlotInertName = '\u0000';
export const QDefaultSlot = '';

/**
Expand All @@ -88,10 +76,6 @@ export const ELEMENT_KEY = 'q:key';
export const ELEMENT_PROPS = 'q:props';
export const ELEMENT_SEQ = 'q:seq';
export const ELEMENT_SEQ_IDX = 'q:seqIdx';
export const ELEMENT_SELF_ID = -1;
export const ELEMENT_ID_SELECTOR = '[q\\:id]';
export const ELEMENT_ID_PREFIX = '#';
export const INLINE_FN_PREFIX = '@';
export const Q_PREFIX = 'q:';

/** Non serializable markers - always begins with `:` character */
Expand Down
6 changes: 1 addition & 5 deletions packages/qwik/src/core/shared/utils/prop.ts
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import { createPropsProxy, type Props, type PropsProxy } from '../jsx/jsx-runtime';
import { _CONST_PROPS, _VAR_PROPS } from './constants';
import { NON_SERIALIZABLE_MARKER_PREFIX, QSlotParent } from './markers';
import { NON_SERIALIZABLE_MARKER_PREFIX } from './markers';

export function isSlotProp(prop: string): boolean {
return !prop.startsWith('q:') && !prop.startsWith(NON_SERIALIZABLE_MARKER_PREFIX);
}

export function isParentSlotProp(prop: string): boolean {
return prop.startsWith(QSlotParent);
}

/** @internal */
export const _restProps = (props: PropsProxy, omit: string[], target: Props = {}) => {
let constPropsTarget: Props | null = null;
Expand Down
4 changes: 2 additions & 2 deletions packages/qwik/src/core/shared/vnode-data-types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ export const VNodeDataChar = {
ID_CHAR: /* ********* */ '=',
PROPS: /* ************** */ 62, // `>` - `q:props' - Component Props
PROPS_CHAR: /* ****** */ '>',
SLOT_REF: /* *********** */ 63, // `?` - `q:sref` - Slot reference.
SLOT_REF_CHAR: /* *** */ '?',
SLOT_PARENT: /* ******** */ 63, // `?` - `q:sparent` - Slot parent.
SLOT_PARENT_CHAR: /* */ '?',
KEY: /* **************** */ 64, // `@` - `q:key` - Element key.
KEY_CHAR: /* ******** */ '@',
SEQ: /* **************** */ 91, // `[` - `q:seq' - Seq value from `useSequentialScope()`
Expand Down
3 changes: 2 additions & 1 deletion packages/qwik/src/core/ssr/ssr-render-jsx.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
QDefaultSlot,
QScopedStyle,
QSlot,
QSlotParent,
qwikInspectorAttr,
} from '../shared/utils/markers';
import { isPromise } from '../shared/utils/promises';
Expand Down Expand Up @@ -192,7 +193,7 @@ function processJSXNode(
if (componentFrame) {
const compId = componentFrame.componentNode.id || '';
const projectionAttrs = isDev ? [DEBUG_TYPE, VirtualType.Projection] : [];
projectionAttrs.push(':', compId);
projectionAttrs.push(QSlotParent, compId);
ssr.openProjection(projectionAttrs);
const host = componentFrame.componentNode;
const node = ssr.getLastNode();
Expand Down
109 changes: 109 additions & 0 deletions packages/qwik/src/core/tests/projection.spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1189,6 +1189,115 @@ describe.each([
);
});

it('should render the same content projection with different structure', async () => {
const Cmp1 = component$(() => {
return (
<>
<h1>Test</h1>
<p>Test content</p>
</>
);
});

const Cmp2 = component$((props: { toggle: boolean }) => {
return (
<>
{props.toggle && <Slot />}
{!props.toggle && (
<>
<Slot />
</>
)}
</>
);
});

const Parent = component$(() => {
const toggle = useSignal(true);
return (
<div>
<button onClick$={() => (toggle.value = !toggle.value)}>toggle</button>
<Cmp2 toggle={toggle.value}>
<Cmp1 />
</Cmp2>
</div>
);
});
const { vNode, document } = await render(<Parent />, { debug: DEBUG });
expect(cleanupAttrs(document.body.querySelector('div')?.outerHTML)).toEqual(
'<div><button>toggle</button><h1>Test</h1><p>Test content</p></div>'
);
expect(vNode).toMatchVDOM(
<Component ssr-required>
<div>
<button>toggle</button>
<Component ssr-required>
<Fragment ssr-required>
<Projection ssr-required>
<Component ssr-required>
<Fragment ssr-required>
<h1>Test</h1>
<p>Test content</p>
</Fragment>
</Component>
</Projection>
</Fragment>
</Component>
</div>
</Component>
);

await trigger(document.body, 'button', 'click');
expect(cleanupAttrs(document.body.querySelector('div')?.outerHTML)).toEqual(
'<div><button>toggle</button><h1>Test</h1><p>Test content</p></div>'
);

expect(vNode).toMatchVDOM(
<Component ssr-required>
<div>
<button>toggle</button>
<Component ssr-required>
<Fragment ssr-required>
<Projection ssr-required>
<Component ssr-required>
<Fragment ssr-required>
<Fragment ssr-required>
<h1>Test</h1>
<p>Test content</p>
</Fragment>
</Fragment>
</Component>
</Projection>
</Fragment>
</Component>
</div>
</Component>
);
await trigger(document.body, 'button', 'click');
expect(cleanupAttrs(document.body.querySelector('div')?.outerHTML)).toEqual(
'<div><button>toggle</button><h1>Test</h1><p>Test content</p></div>'
);
expect(vNode).toMatchVDOM(
<Component ssr-required>
<div>
<button>toggle</button>
<Component ssr-required>
<Fragment ssr-required>
<Projection ssr-required>
<Component ssr-required>
<Fragment ssr-required>
<h1>Test</h1>
<p>Test content</p>
</Fragment>
</Component>
</Projection>
</Fragment>
</Component>
</div>
</Component>
);
});

describe('ensureProjectionResolved', () => {
(globalThis as any).log = [] as string[];
beforeEach(() => {
Expand Down
1 change: 0 additions & 1 deletion packages/qwik/src/server/qwik-copy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export {
QScopedStyle,
QSlot,
QSlotParent,
QSlotRef,
QStyle,
QTemplate,
QVersionAttr,
Expand Down
Loading

0 comments on commit d82f44a

Please sign in to comment.