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(v2): fix toggle named projection #6563

Merged
merged 5 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
4 changes: 2 additions & 2 deletions packages/qwik/src/core/v2/client/dom-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ import {
vnode_setProp,
type VNodeJournal,
} from './vnode';
import { vnode_diff } from './vnode-diff';
import { isSlotProp, vnode_diff } from './vnode-diff';
Copy link
Member

Choose a reason for hiding this comment

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

are we sure we want isSlotProp to sit @ vnode-diff? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I will think about it later

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this into separate file


/** @public */
export function getDomContainer(element: Element | ElementVNode): IClientContainer {
Expand Down Expand Up @@ -289,7 +289,7 @@ export class DomContainer extends _SharedContainer implements IClientContainer,
vNode[VNodeProps.flags] |= VNodeFlags.Resolved;
for (let i = vnode_getPropStartIndex(vNode); i < vNode.length; i = i + 2) {
const prop = vNode[i] as string;
if (!prop.startsWith('q:')) {
if (isSlotProp(prop)) {
Copy link
Member

Choose a reason for hiding this comment

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

👨‍🍳 💋

const value = vNode[i + 1];
if (typeof value == 'string') {
vNode[i + 1] = this.$vnodeLocate$(value);
Expand Down
32 changes: 24 additions & 8 deletions packages/qwik/src/core/v2/client/vnode-diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ import {
vnode_setProp,
vnode_setText,
vnode_truncate,
vnode_walkVNode,
type VNodeJournal,
} from './vnode';
import { getNewElementNamespaceData } from './vnode-namespace';
Expand Down Expand Up @@ -396,7 +397,7 @@ export const vnode_diff = (
// we need to create empty projections for all the slots to remove unused slots content
for (let i = vnode_getPropStartIndex(host); i < host.length; i = i + 2) {
const prop = host[i] as string;
if (!prop.startsWith('q:')) {
if (isSlotProp(prop)) {
const slotName = prop;
projections.push(slotName);
projections.push(createProjectionJSXNode(slotName));
Expand Down Expand Up @@ -1027,7 +1028,7 @@ export const vnode_diff = (
if (host) {
for (let i = vnode_getPropStartIndex(host); i < host.length; i = i + 2) {
const prop = host[i] as string;
if (!prop.startsWith('q:')) {
if (isSlotProp(prop)) {
const value = host[i + 1];
container.setHostProp(vNewNode, prop, value);
}
Expand Down Expand Up @@ -1189,8 +1190,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) {
const attrs = vCursor;
for (let i = VirtualVNodeProps.PROPS_OFFSET; i < attrs.length; i = i + 2) {
const key = attrs[i] as string;
if (!key.startsWith(':') && !key.startsWith('q:')) {
// any prop which does not start with `:` or `q:` is a content-projection prop.
if (!isParentSlotProp(key) && isSlotProp(key)) {
const value = attrs[i + 1];
if (value) {
attrs[i + 1] = null; // prevent infinite loop
Expand Down Expand Up @@ -1220,6 +1220,17 @@ export function cleanup(container: ClientContainer, vNode: VNode) {
vCursor = vFirstChild;
continue;
}
} else if (vCursor === vNode) {
/**
* If it is a projection and we are at the root, then we should only walk the children to
* materialize the projection content. This is because we could have references in the vnode
* refs map which need to be materialized before cleanup.
*/
const vFirstChild = vnode_getFirstChild(vCursor);
if (vFirstChild) {
vnode_walkVNode(vFirstChild);
return;
}
}
}
// Out of children
Expand All @@ -1233,10 +1244,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) {
vCursor = vNextSibling;
continue;
}
if (vCursor === vNode) {
// we are back where we started, we are done.
return;
}

// Out of siblings, go to parent
vParent = vnode_getParent(vCursor);
while (vParent) {
Expand Down Expand Up @@ -1274,6 +1282,14 @@ function cleanupStaleUnclaimedProjection(journal: VNodeJournal, projection: VNod
}
}

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

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

/**
* This marks the property as immutable. It is needed for the QRLs so that QwikLoader can get a hold
* of them. This character must be `:` so that the `vnode_getAttr` can ignore them.
Expand Down
4 changes: 0 additions & 4 deletions packages/qwik/src/core/v2/client/vnode-namespace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,6 @@ function vnode_cloneElementWithNamespace(
vCursor = vNextSibling;
continue;
}
if (vCursor === elementVNode) {
// we are back where we started, we are done.
return rootElement;
}
// Out of siblings, go to parent
vParent = vnode_getParent(vCursor);
while (vParent) {
Expand Down
51 changes: 49 additions & 2 deletions packages/qwik/src/core/v2/client/vnode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,53 @@ export const vnode_ensureElementInflated = (vnode: VNode) => {
}
};

/** Walks the VNode tree and materialize it using `vnode_getFirstChild`. */
export function vnode_walkVNode(vNode: VNode) {
let vCursor: VNode | null = vNode;
// Depth first traversal
if (vnode_isTextVNode(vNode)) {
// Text nodes don't have subscriptions or children;
return;
}
let vParent: VNode | null = null;
do {
const vFirstChild = vnode_getFirstChild(vCursor);
if (vFirstChild) {
vCursor = vFirstChild;
continue;
}
// Out of children
if (vCursor === vNode) {
// we are where we started, this means that vNode has no children, so we are done.
return;
}
// Out of children, go to next sibling
const vNextSibling = vnode_getNextSibling(vCursor);
if (vNextSibling) {
vCursor = vNextSibling;
continue;
}
// Out of siblings, go to parent
vParent = vnode_getParent(vCursor);
while (vParent) {
if (vParent === vNode) {
// We are back where we started, we are done.
return;
}
const vNextParentSibling = vnode_getNextSibling(vParent);
if (vNextParentSibling) {
vCursor = vNextParentSibling;
break;
}
vParent = vnode_getParent(vParent);
}
if (vParent == null) {
// We are done.
return;
}
} while (true as boolean);
}

export function vnode_getDOMChildNodes(
journal: VNodeJournal,
root: VNode,
Expand Down Expand Up @@ -614,10 +661,10 @@ export const vnode_locate = (rootVNode: ElementVNode, id: string | Element): VNo
refElement = id;
}
assertDefined(refElement, 'Missing refElement.');
if (!Array.isArray(refElement)) {
if (!vnode_isVNode(refElement)) {
assertTrue(
containerElement.contains(refElement),
'refElement must be a child of containerElement.'
`Couldn't find the element inside the container while locating the VNode.`
);
// We need to find the vnode.
let parent = refElement;
Expand Down
3 changes: 1 addition & 2 deletions packages/qwik/src/core/v2/tests/projection.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1865,8 +1865,7 @@ describe.each([
);
});

// TODO(slot): fix this test
it.skip('#2688 - case 2', async () => {
it('#2688 - case 2', async () => {
const Switch = component$((props: { name: string }) => {
return <Slot name={props.name} />;
});
Expand Down
3 changes: 1 addition & 2 deletions starters/e2e/e2e.slot.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,7 @@ test.describe("slot", () => {
await expect(modalContent).not.toBeHidden();
});

// TODO(v2): fix this
test.skip("issue 2688", async ({ page }) => {
test("issue 2688", async ({ page }) => {
const result = page.locator("#issue-2688-result");
const button = page.locator("#issue-2688-button");
const count = page.locator("#btn-count");
Expand Down
Loading