fix(v2): fix streaming tests #6553
Conversation
Co-authored-by: Shai Reznik <shairez@users.noreply.github.com> Co-authored-by: Igal Klebanov <igalklebanov@gmail.com> Co-authored-by: Naor Peled <me@naor.dev>
| @@ -1019,14 +1025,12 @@ class SSRContainer extends _SharedContainer implements ISSRContainer { | |||
| const frame: ContainerElementFrame = { | |||
| tagNesting: tagNesting, | |||
| parent: this.currentElementFrame, | |||
| elementName: tag, | |||
| elementName: elementName, | |||
There was a problem hiding this comment.
| elementName: elementName, | |
| elementName, |
remove waitForTimeout from test
| /** | ||
| * @param tag | ||
| * @param elementName | ||
| * @param depthFirstElementIdx | ||
| * @param isElement | ||
| */ |
There was a problem hiding this comment.
let's align with current function's arguments..
why do we even need this js docs block? seems to not add much.
| /** @param tag Returns true if `qDispatchEvent` needs patching */ | ||
| function createNewElement(jsx: JSXNode, tag: string): boolean { | ||
| const element = createElementWithNamespace(tag); | ||
| /** @param elementName Returns true if `qDispatchEvent` needs patching */ |
There was a problem hiding this comment.
this is misleading.
the return value is only affected by jsx argument's constProps. am I missing something?
let's change it to something like this?
| /** @param elementName Returns true if `qDispatchEvent` needs patching */ | |
| /** | |
| * Returns whether `qDispatchEvent` needs patching. This is true when one of the `jsx` argument's const props has the name of an event. | |
| * @returns {boolean} | |
| */ |
There was a problem hiding this comment.
This is because when we create an element we handle only the constProps. The varProps are handled later here needsQDispatchEventPatch = setBulkProps(vNode, jsxAttrs) || needsQDispatchEventPatch;
There was a problem hiding this comment.
Yeah, but the jsdoc is for function createNewElement(jsx: JSXNode, elementName: string): boolean {. In which needsQDispatchEventPatch is only set to true once - when isJsxPropertyAnEventName(key) is true.
There was a problem hiding this comment.
yes, I will change the description
|
@igalklebanov @Varixo please write a comment when this PR is ready to be merged, thanks! |
For me it is ready |
Yalla let's go! 🚀 |
this PR fixes the streaming tests + some refactoring