Skip to content
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

fix: finding context parent and sorting projections in the scheduler #7204

Open
wants to merge 1 commit into
base: build/v2
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/loud-dolphins-relate.md
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
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
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
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
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
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
Loading