Skip to content

Commit

Permalink
Merge pull request #7204 from QwikDev/v2-find-context-error
Browse files Browse the repository at this point in the history 
fix: finding context parent and sorting projections in the scheduler
  • Loading branch information
@wmertens
wmertens authored Jan 3, 2025
2 parents 9b71fc0 + 58ba5b4 commit 6ed1284
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 37 deletions.
5 changes: 5 additions & 0 deletions .changeset/loud-dolphins-relate.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@qwik.dev/core': patch
---

fix: finding context parent and sorting projections in the scheduler
13 changes: 6 additions & 7 deletions packages/qwik/src/core/client/dom-container.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,14 +251,13 @@ export class DomContainer extends _SharedContainer implements IClientContainer {
if (vnode_getProp(vNode, OnRenderProp, null) !== null) {
return vNode as any as HostElement;
}
// If virtual node, than it could be a slot so we need to read its parent.
const parent = vnode_getProp<VNode>(vNode, QSlotParent, this.$vnodeLocate$);
if (parent) {
vNode = parent;
continue;
}
vNode =
vnode_getParent(vNode) ||
// If virtual node, than it could be a slot so we need to read its parent.
vnode_getProp<VNode>(vNode, QSlotParent, this.$vnodeLocate$);
} else {
vNode = vnode_getParent(vNode);
}
vNode = vnode_getParent(vNode);
}
return null;
}
Expand Down
30 changes: 20 additions & 10 deletions packages/qwik/src/core/client/vnode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1889,21 +1889,29 @@ export const vnode_getType = (vnode: VNode): 1 | 3 | 11 => {
const isElement = (node: any): node is Element =>
node && typeof node == 'object' && fastNodeType(node) === /** Node.ELEMENT_NODE* */ 1;

/// These global variables are used to avoid creating new arrays for each call to `vnode_getPathToClosestDomNode`.
/// These global variables are used to avoid creating new arrays for each call to `vnode_documentPosition`.
const aPath: VNode[] = [];
const bPath: VNode[] = [];
export const vnode_documentPosition = (a: VNode, b: VNode): -1 | 0 | 1 => {
export const vnode_documentPosition = (
a: VNode,
b: VNode,
rootVNode: ElementVNode | null
): -1 | 0 | 1 => {
if (a === b) {
return 0;
}

let aDepth = -1;
let bDepth = -1;
while (a) {
a = (aPath[++aDepth] = a)[VNodeProps.parent]!;
const vNode = (aPath[++aDepth] = a);
a = (vNode[VNodeProps.parent] ||
(rootVNode && vnode_getProp(a, QSlotParent, (id) => vnode_locate(rootVNode, id))))!;
}
while (b) {
b = (bPath[++bDepth] = b)[VNodeProps.parent]!;
const vNode = (bPath[++bDepth] = b);
b = (vNode[VNodeProps.parent] ||
(rootVNode && vnode_getProp(b, QSlotParent, (id) => vnode_locate(rootVNode, id))))!;
}

while (aDepth >= 0 && bDepth >= 0) {
Expand All @@ -1929,6 +1937,11 @@ export const vnode_documentPosition = (a: VNode, b: VNode): -1 | 0 | 1 => {
return -1;
}
} while (cursor);
if (rootVNode && vnode_getProp(b, QSlotParent, (id) => vnode_locate(rootVNode, id))) {
// The "b" node is a projection, so we need to set it after "a" node,
// because the "a" node could be a context provider.
return -1;
}
// The node is not in the list of siblings, that means it must be disconnected.
return 1;
}
Expand Down Expand Up @@ -1967,12 +1980,9 @@ export const vnode_getProjectionParentComponent = (
vHost &&
(vnode_isVirtualVNode(vHost) ? vnode_getProp(vHost, OnRenderProp, null) === null : true)
) {
const qSlotParentProp = vnode_getProp(vHost, QSlotParent, null) as string | VNode | null;
const qSlotParent =
qSlotParentProp &&
(typeof qSlotParentProp === 'string'
? vnode_locate(rootVNode, qSlotParentProp)
: qSlotParentProp);
const qSlotParent = vnode_getProp<VNode | null>(vHost, QSlotParent, (id) =>
vnode_locate(rootVNode, id)
);
const vProjectionParent = vnode_isVirtualVNode(vHost) && qSlotParent;
if (vProjectionParent) {
// We found a projection, so we need to go up one more level.
Expand Down
12 changes: 6 additions & 6 deletions packages/qwik/src/core/client/vnode.unit.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,25 +655,25 @@ describe('vnode', () => {
parent.innerHTML = '<b></b><i></i>';
const b = vnode_getFirstChild(vParent) as ElementVNode;
const i = vnode_getNextSibling(b) as ElementVNode;
expect(vnode_documentPosition(b, i)).toBe(-1);
expect(vnode_documentPosition(i, b)).toBe(1);
expect(vnode_documentPosition(b, i, null)).toBe(-1);
expect(vnode_documentPosition(i, b, null)).toBe(1);
});
it('should compare two virtual vNodes', () => {
parent.innerHTML = 'AB';
document.qVNodeData.set(parent, '{B}{B}');
const a = vnode_getFirstChild(vParent) as ElementVNode;
const b = vnode_getNextSibling(a) as ElementVNode;
expect(vnode_documentPosition(a, b)).toBe(-1);
expect(vnode_documentPosition(b, a)).toBe(1);
expect(vnode_documentPosition(a, b, null)).toBe(-1);
expect(vnode_documentPosition(b, a, null)).toBe(1);
});
it('should compare two virtual vNodes', () => {
parent.innerHTML = 'AB';
document.qVNodeData.set(parent, '{{B}}{B}');
const a = vnode_getFirstChild(vParent) as ElementVNode;
const a2 = vnode_getFirstChild(a) as ElementVNode;
const b = vnode_getNextSibling(a) as ElementVNode;
expect(vnode_documentPosition(a2, b)).toBe(-1);
expect(vnode_documentPosition(b, a2)).toBe(1);
expect(vnode_documentPosition(a2, b, null)).toBe(-1);
expect(vnode_documentPosition(b, a2, null)).toBe(1);
});
});
});
45 changes: 32 additions & 13 deletions packages/qwik/src/core/shared/scheduler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ export const createScheduler = (
};
chore.$promise$ = new Promise((resolve) => (chore.$resolve$ = resolve));
DEBUG && debugTrace('schedule', chore, currentChore, choreQueue);
chore = sortedInsert(choreQueue, chore);
chore = sortedInsert(choreQueue, chore, (container as DomContainer).rootVNode || null);
if (!journalFlushScheduled && runLater) {
// If we are not currently draining, we need to schedule a drain.
journalFlushScheduled = true;
Expand All @@ -274,7 +274,7 @@ export const createScheduler = (
if (runLater) {
return chore.$promise$;
} else {
return drainUpTo(chore);
return drainUpTo(chore, (container as DomContainer).rootVNode || null);
}
}

Expand All @@ -283,7 +283,7 @@ export const createScheduler = (
*
* @param runUptoChore
*/
function drainUpTo(runUptoChore: Chore): ValueOrPromise<unknown> {
function drainUpTo(runUptoChore: Chore, rootVNode: ElementVNode | null): ValueOrPromise<unknown> {
// If it already ran, it's not in the queue
if (runUptoChore.$executed$) {
return runUptoChore.$returnValue$;
Expand All @@ -294,7 +294,7 @@ export const createScheduler = (
}
while (choreQueue.length) {
const nextChore = choreQueue.shift()!;
const order = choreComparator(nextChore, runUptoChore, false);
const order = choreComparator(nextChore, runUptoChore, rootVNode, false);
if (order === null) {
continue;
}
Expand All @@ -313,7 +313,7 @@ export const createScheduler = (
}
const returnValue = executeChore(nextChore);
if (isPromise(returnValue)) {
const promise = returnValue.then(() => drainUpTo(runUptoChore));
const promise = returnValue.then(() => drainUpTo(runUptoChore, rootVNode));
return promise;
}
}
Expand Down Expand Up @@ -466,9 +466,24 @@ function vNodeAlreadyDeleted(chore: Chore): boolean {
* @returns A number indicating the relative order of the chores, or null if invalid. A negative
* number means `a` runs before `b`.
*/
function choreComparator(a: Chore, b: Chore, shouldThrowOnHostMismatch: true): number;
function choreComparator(a: Chore, b: Chore, shouldThrowOnHostMismatch: false): number | null;
function choreComparator(a: Chore, b: Chore, shouldThrowOnHostMismatch: boolean): number | null {
function choreComparator(
a: Chore,
b: Chore,
rootVNode: ElementVNode | null,
shouldThrowOnHostMismatch: true
): number;
function choreComparator(
a: Chore,
b: Chore,
rootVNode: ElementVNode | null,
shouldThrowOnHostMismatch: false
): number | null;
function choreComparator(
a: Chore,
b: Chore,
rootVNode: ElementVNode | null,
shouldThrowOnHostMismatch: boolean
): number | null {
const macroTypeDiff = (a.$type$ & ChoreType.MACRO) - (b.$type$ & ChoreType.MACRO);
if (macroTypeDiff !== 0) {
return macroTypeDiff;
Expand All @@ -483,7 +498,7 @@ function choreComparator(a: Chore, b: Chore, shouldThrowOnHostMismatch: boolean)
if (aHost !== bHost && aHost !== null && bHost !== null) {
if (vnode_isVNode(aHost) && vnode_isVNode(bHost)) {
// we are running on the client.
const hostDiff = vnode_documentPosition(aHost, bHost);
const hostDiff = vnode_documentPosition(aHost, bHost, rootVNode);
if (hostDiff !== 0) {
return hostDiff;
}
Expand Down Expand Up @@ -530,15 +545,19 @@ function choreComparator(a: Chore, b: Chore, shouldThrowOnHostMismatch: boolean)
return 0;
}

function sortedFindIndex(sortedArray: Chore[], value: Chore): number {
function sortedFindIndex(
sortedArray: Chore[],
value: Chore,
rootVNode: ElementVNode | null
): number {
/// We need to ensure that the `queue` is sorted by priority.
/// 1. Find a place where to insert into.
let bottom = 0;
let top = sortedArray.length;
while (bottom < top) {
const middle = bottom + ((top - bottom) >> 1);
const midChore = sortedArray[middle];
const comp = choreComparator(value, midChore, true);
const comp = choreComparator(value, midChore, rootVNode, true);
if (comp < 0) {
top = middle;
} else if (comp > 0) {
Expand All @@ -551,10 +570,10 @@ function sortedFindIndex(sortedArray: Chore[], value: Chore): number {
return ~bottom;
}

function sortedInsert(sortedArray: Chore[], value: Chore): Chore {
function sortedInsert(sortedArray: Chore[], value: Chore, rootVNode: ElementVNode | null): Chore {
/// We need to ensure that the `queue` is sorted by priority.
/// 1. Find a place where to insert into.
const idx = sortedFindIndex(sortedArray, value);
const idx = sortedFindIndex(sortedArray, value, rootVNode);
if (idx < 0) {
/// 2. Insert the chore into the queue.
sortedArray.splice(~idx, 0, value);
Expand Down
50 changes: 49 additions & 1 deletion packages/qwik/src/core/tests/use-context.spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ import {
ssrRenderToDom,
trigger,
} from '@qwik.dev/core/testing';
import { describe, expect, it } from 'vitest';
import { describe, expect, it, vi } from 'vitest';
import type { Signal } from '../signal/signal.public';
import { ErrorProvider } from '../../testing/rendering.unit-util';
import * as qError from '../shared/error/error';

const debug = false; //true;
Error.stackTraceLimit = 100;
Expand Down Expand Up @@ -99,6 +101,52 @@ describe.each([
);
});

it('should find context parent from Slot inside Slot', async () => {
const qErrorSpy = vi.spyOn(qError, 'qError');
const contextId = createContextId('contextId');

const ContextProducer = component$(() => {
const context = {
disabled: false,
};
useContextProvider(contextId, context);
return <Slot />;
});

const ProducerParent = component$(() => {
return (
<ContextProducer>
<Slot />
</ContextProducer>
);
});

const ContextConsumer = component$(() => {
useContext(contextId);
return <Slot />;
});

const Parent = component$(() => {
return (
<ProducerParent>
<ContextConsumer />
</ProducerParent>
);
});

try {
await render(
<ErrorProvider>
<Parent />
</ErrorProvider>,
{ debug }
);
expect(qErrorSpy).not.toHaveBeenCalled();
} catch (e) {
expect(qErrorSpy).not.toHaveBeenCalled();
}
});

describe('regression', () => {
it('#4038', async () => {
interface IMyComponent {
Expand Down

0 comments on commit 6ed1284

Please sign in to comment.