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.

Sign up for GitHub

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 vnode separators #6596

Merged
merged 3 commits into from
Jun 25, 2024
Merged

fix(v2): fix vnode separators #6596

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d01bc8
fix(v2): fix vnode separators
Varixo Jun 24, 2024
d8a1a58
add vnode encoding for ELEMENT_SEQ_IDX
Varixo Jun 24, 2024
592316e
fix vnode separators
Varixo Jun 25, 2024
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
8 changes: 3 additions & 5 deletions packages/qwik/src/core/use/use-sequential-scope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { verifySerializable } from '../state/common';
import { getContext, type QContext } from '../state/context';
import { ELEMENT_SEQ } from '../util/markers';
import { ELEMENT_SEQ, ELEMENT_SEQ_IDX } from '../util/markers';
import { qDev, qSerialize } from '../util/qdev';
import type { fixMeAny, HostElement } from '../v2/shared/types';
import { useInvokeContext, type RenderInvokeContext } from './use-core';
Expand Down Expand Up @@ -31,11 +31,11 @@ export const useSequentialScope = <T>(): SequentialScope<T> => {
seq = [];
iCtx.$container2$.setHostProp(host, ELEMENT_SEQ, seq);
}
let seqIdx = iCtx.$container2$.getHostProp<number>(host, SEQ_IDX_LOCAL);
let seqIdx = iCtx.$container2$.getHostProp<number>(host, ELEMENT_SEQ_IDX);
if (seqIdx === null) {
seqIdx = 0;
}
iCtx.$container2$.setHostProp(host, SEQ_IDX_LOCAL, seqIdx + 1);
iCtx.$container2$.setHostProp(host, ELEMENT_SEQ_IDX, seqIdx + 1);
while (seq.length <= seqIdx) {
seq.push(undefined);
}
Expand Down Expand Up @@ -73,5 +73,3 @@ export const useSequentialScope = <T>(): SequentialScope<T> => {
};
}
};

export const SEQ_IDX_LOCAL = 'q:seqIdx';
1 change: 1 addition & 0 deletions packages/qwik/src/core/util/markers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export const ELEMENT_ID = 'q:id';
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 = '#';
Expand Down
4 changes: 2 additions & 2 deletions packages/qwik/src/core/v2/client/dom-container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import { ERROR_CONTEXT, isRecoverable } from '../../render/error-handling';
import type { JSXOutput } from '../../render/jsx/types/jsx-node';
import type { StoreTracker } from '../../state/store';
import type { ContextId } from '../../use/use-context';
import { SEQ_IDX_LOCAL } from '../../use/use-sequential-scope';
import { EMPTY_ARRAY } from '../../util/flyweight';
import { throwErrorAndStop } from '../../util/log';
import {
ELEMENT_PROPS,
ELEMENT_SEQ,
ELEMENT_SEQ_IDX,
OnRenderProp,
QContainerAttr,
QContainerSelector,
Expand Down Expand Up @@ -263,7 +263,7 @@ export class DomContainer extends _SharedContainer implements IClientContainer,
case QCtxAttr:
getObjectById = this.$getObjectById$;
break;
case SEQ_IDX_LOCAL:
case ELEMENT_SEQ_IDX:
getObjectById = parseInt;
break;
}
Expand Down
1 change: 0 additions & 1 deletion packages/qwik/src/core/v2/client/process-vnode-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,6 @@ export function processVNodeData(document: Document) {
ch = VNodeDataSeparator.ADVANCE_1;
}
}
vData_end = vData_start;
vData_end = findVDataSectionEnd(vData, vData_start, vData_length);
} else {
vNodeElementIndex = Number.MAX_SAFE_INTEGER;
Expand Down
3 changes: 3 additions & 0 deletions packages/qwik/src/core/v2/client/vnode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ import {
ELEMENT_KEY,
ELEMENT_PROPS,
ELEMENT_SEQ,
ELEMENT_SEQ_IDX,
OnRenderProp,
QContainerAttr,
QContainerAttrEnd,
Expand Down Expand Up @@ -1688,6 +1689,8 @@ function materializeFromVNodeData(
vnode_setAttr(null, vParent, ELEMENT_KEY, consumeValue());
} else if (peek() === VNodeDataChar.SEQ) {
vnode_setAttr(null, vParent, ELEMENT_SEQ, consumeValue());
} else if (peek() === VNodeDataChar.SEQ_IDX) {
vnode_setAttr(null, vParent, ELEMENT_SEQ_IDX, consumeValue());
} else if (peek() === VNodeDataChar.CONTEXT) {
vnode_setAttr(null, vParent, QCtxAttr, consumeValue());
} else if (peek() === VNodeDataChar.OPEN) {
Expand Down
5 changes: 2 additions & 3 deletions packages/qwik/src/core/v2/shared/component-execution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@ import type { JSXNode, JSXOutput } from '../../render/jsx/types/jsx-node';
import { SubscriptionType } from '../../state/common';
import { invokeApply, newInvokeContext } from '../../use/use-core';
import { USE_ON_LOCAL, type UseOnMap } from '../../use/use-on';
import { SEQ_IDX_LOCAL } from '../../use/use-sequential-scope';
import { EMPTY_OBJ } from '../../util/flyweight';
import { ELEMENT_PROPS, OnRenderProp, RenderEvent } from '../../util/markers';
import { ELEMENT_PROPS, ELEMENT_SEQ_IDX, OnRenderProp, RenderEvent } from '../../util/markers';
import { isPromise, safeCall } from '../../util/promises';
import type { ValueOrPromise } from '../../util/types';
import type { Container2, HostElement, fixMeAny } from './types';
Expand Down Expand Up @@ -73,7 +72,7 @@ export const executeComponent2 = (
const executeComponentWithPromiseExceptionRetry = (): ValueOrPromise<JSXOutput> =>
safeCall<JSXOutput, JSXOutput, JSXOutput>(
() => {
container.setHostProp(renderHost, SEQ_IDX_LOCAL, null);
container.setHostProp(renderHost, ELEMENT_SEQ_IDX, null);
container.setHostProp(renderHost, ELEMENT_PROPS, props);
return componentFn(props);
},
Expand Down
12 changes: 7 additions & 5 deletions packages/qwik/src/core/v2/shared/vnode-data-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ export const VNodeDataSeparator = {
ADVANCE_16: /* ******** */ 37, // `%` is vNodeData separator skipping 8.
ADVANCE_32_CH: /* **** */ `&`, // `&` is vNodeData separator skipping 16.
ADVANCE_32: /* ******** */ 38, // `&` is vNodeData separator skipping 16.
ADVANCE_64_CH: /* **** */ '`', // '`'` is vNodeData separator skipping 32.
ADVANCE_64: /* ******** */ 39, // '`'` is vNodeData separator skipping 32.
ADVANCE_64_CH: /* **** */ `'`, // `'` is vNodeData separator skipping 32.
ADVANCE_64: /* ******** */ 39, // `'` is vNodeData separator skipping 32.
ADVANCE_128_CH: /* *** */ `(`, // `(` is vNodeData separator skipping 64.
ADVANCE_128: /* ******* */ 40, // `(` is vNodeData separator skipping 64.
ADVANCE_256_CH: /* *** */ `)`, // `)` is vNodeData separator skipping 128.
Expand All @@ -34,12 +34,12 @@ export const VNodeDataSeparator = {
ADVANCE_512: /* ******* */ 42, // `*` is vNodeData separator skipping 256.
ADVANCE_1024_CH: /* ** */ `+`, // `+` is vNodeData separator skipping 512.
ADVANCE_1024: /* ****** */ 43, // `+` is vNodeData separator skipping 512.
ADVANCE_2048_CH: /* * */ '`', // '`'` is vNodeData separator skipping 1024.
ADVANCE_2048: /* ****** */ 44, // '`'` is vNodeData separator skipping 1024.
ADVANCE_2048_CH: /* * */ ',', // ',' is vNodeData separator skipping 1024.
ADVANCE_2048: /* ****** */ 44, // ',' is vNodeData separator skipping 1024.
ADVANCE_4096_CH: /* * */ `.`, // `.` is vNodeData separator skipping 2048.
ADVANCE_4096: /* ****** */ 46, // `.` is vNodeData separator skipping 2048.
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 don't know why we are missing the 45 charcode?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is weird! I don't think we should be skipping anything. I think that is a bug and should be fixed.

ADVANCE_8192_CH: /* * */ `/`, // `/` is vNodeData separator skipping 4096.
ADVANCE_9102: /* ****** */ 47, // `/` is vNodeData separator skipping 4096.
ADVANCE_8192: /* ****** */ 47, // `/` is vNodeData separator skipping 4096.
};

/** VNodeDataChar contains information about the VNodeData used for encoding props */
Expand Down Expand Up @@ -67,6 +67,8 @@ export const VNodeDataChar = {
DON_T_USE_CHAR: '\\',
CONTEXT: /* ************ */ 93, // `]` - `q:ctx' - Component context/props
CONTEXT_CHAR: /* **** */ ']',
SEQ_IDX: /* ************ */ 94, // `[` - `q:seqIdx' - Sequential scope id
SEQ_IDX_CHAR: /* **** */ '^',
SEPARATOR: /* ********* */ 124, // `|` - Separator char to encode any key/value pairs.
SEPARATOR_CHAR: /* ** */ '|',
SLOT: /* ************** */ 126, // `~` - `q:slot' - Slot name
Expand Down
1 change: 1 addition & 0 deletions packages/qwik/src/server/qwik-copy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export {
ELEMENT_KEY,
ELEMENT_PROPS,
ELEMENT_SEQ,
ELEMENT_SEQ_IDX,
OnRenderProp,
QContainerAttr,
QCtxAttr,
Expand Down
8 changes: 6 additions & 2 deletions packages/qwik/src/server/v2-ssr-container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
ELEMENT_KEY,
ELEMENT_PROPS,
ELEMENT_SEQ,
ELEMENT_SEQ_IDX,
OnRenderProp,
QCtxAttr,
QScopedStyle,
Expand Down Expand Up @@ -621,7 +622,7 @@ class SSRContainer extends _SharedContainer implements ISSRContainer {
if (flag !== VNodeDataFlag.NONE) {
lastSerializedIdx = this.emitVNodeSeparators(lastSerializedIdx, elementIdx);
if (flag & VNodeDataFlag.REFERENCE) {
this.write('~');
this.write(VNodeDataSeparator.REFERENCE_CH);
}
if (flag & (VNodeDataFlag.TEXT_DATA | VNodeDataFlag.VIRTUAL_NODE)) {
let fragmentAttrs: SsrAttrs | null = null;
Expand Down Expand Up @@ -701,6 +702,9 @@ class SSRContainer extends _SharedContainer implements ISSRContainer {
case ELEMENT_SEQ:
write(VNodeDataChar.SEQ_CHAR);
break;
case ELEMENT_SEQ_IDX:
write(VNodeDataChar.SEQ_IDX_CHAR);
break;
// Skipping `\` character for now because it is used for escaping.
case QCtxAttr:
write(VNodeDataChar.CONTEXT_CHAR);
Expand Down Expand Up @@ -956,7 +960,7 @@ class SSRContainer extends _SharedContainer implements ISSRContainer {
let skipCount = elementIdx - lastSerializedIdx;
// console.log('emitVNodeSeparators', lastSerializedIdx, elementIdx, skipCount);
while (skipCount != 0) {
if (skipCount >= 4096) {
Copy link
Member Author

@Varixo Varixo Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@mhevery I think this is a good change. If skipCount is equal to 4096 then we want to execute code in the else block, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that is correct. Thank you for noticing.

if (skipCount > 4096) {
this.write(VNodeDataSeparator.ADVANCE_8192_CH);
skipCount -= 8192;
} else {
Expand Down
Loading