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: the use hook didn't work when type is Slot. #7250

Open
wants to merge 21 commits into
base: build/v2
Choose a base branch
from
Open
5 changes: 5 additions & 0 deletions .changeset/old-mayflies-fetch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@qwik.dev/core': patch
---

fix: the use hook didn't work when type is Slot.
13 changes: 10 additions & 3 deletions packages/qwik/src/core/shared/component-execution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { isDev } from '@qwik.dev/core/build';
import { isQwikComponent, type OnRenderFn } from './component.public';
import { assertDefined } from './error/assert';
import { isQrl, type QRLInternal } from './qrl/qrl-class';
import { JSXNodeImpl, isJSXNode, type Props } from './jsx/jsx-runtime';
import type { JSXNodeInternal, JSXOutput } from './jsx/types/jsx-node';
import { Fragment, JSXNodeImpl, _jsxSorted, isJSXNode, type Props } from './jsx/jsx-runtime';
import type { FunctionComponent, JSXNodeInternal, JSXOutput } from './jsx/types/jsx-node';
import type { KnownEventNames } from './jsx/types/jsx-qwik-events';
import { invokeApply, newInvokeContext, untrack } from '../use/use-core';
import { type EventQRL, type UseOnMap } from '../use/use-on';
Expand All @@ -23,6 +23,7 @@ import { logWarn } from './utils/log';
import { EffectProperty, isSignal } from '../signal/signal';
import { vnode_isVNode } from '../client/vnode';
import { clearVNodeEffectDependencies } from '../signal/signal-subscriber';
import { Slot } from '../shared/jsx/slot.public';

/**
* Use `executeComponent` to execute a component.
Expand Down Expand Up @@ -101,7 +102,13 @@ export const executeComponent = (
(jsx) => {
const useOnEvents = container.getHostProp<UseOnMap>(renderHost, USE_ON_LOCAL);
if (useOnEvents) {
return maybeThen(addUseOnEvents(jsx, useOnEvents), () => jsx);
let _jsx = jsx;
if (_jsx && (_jsx as JSXNodeInternal<FunctionComponent>).type === Slot) {
_jsx = _jsxSorted(Fragment, null, null, [jsx], 0, null);
}
Varixo marked this conversation as resolved.
Show resolved Hide resolved
return maybeThen(addUseOnEvents(_jsx, useOnEvents), () => {
return _jsx;
});
}
return jsx;
},
Expand Down
4 changes: 3 additions & 1 deletion packages/qwik/src/core/ssr/ssr-render-jsx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ function processJSXNode(
const node = ssr.getLastNode();
const slotName = getSlotName(host, jsx, ssr);
projectionAttrs.push(QSlot, slotName);

enqueue(new ParentComponentData(options.styleScoped, options.parentComponentFrame));
enqueue(ssr.closeProjection);
const slotDefaultChildren: JSXChildren | null = jsx.children || null;
Expand Down Expand Up @@ -300,7 +301,8 @@ function processJSXNode(
options.styleScoped,
options.parentComponentFrame
);
const jsxOutput = applyQwikComponentBody(ssr, jsx, type);

const jsxOutput: any = applyQwikComponentBody(ssr, jsx, type);
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove any type here?

const compStyleComponentId = addComponentStylePrefix(host.getProp(QScopedStyle));
enqueue(new ParentComponentData(options.styleScoped, options.parentComponentFrame));
enqueue(ssr.closeComponent);
Expand Down
76 changes: 75 additions & 1 deletion packages/qwik/src/core/tests/use-on.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ import {
$,
Fragment as Awaited,
Fragment as Component,
Fragment as Projection,
component$,
Fragment,
Fragment as Signal,
Slot,
useOn,
useOnDocument,
useOnWindow,
Expand All @@ -21,7 +23,7 @@ Error.stackTraceLimit = 100;

describe.each([
{ render: ssrRenderToDom }, //
{ render: domRender }, //
{ render: domRender }, ///
Copy link
Member

Choose a reason for hiding this comment

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

Remove additional /

])('$render.name: useOn', ({ render }) => {
it('should update value', async () => {
const Counter = component$((props: { initial: number }) => {
Expand Down Expand Up @@ -647,4 +649,76 @@ describe.each([
await trigger(document.body, 'div', 'click');
await expect(document.querySelector('div')).toMatchDOM(<div>1</div>);
});

it('issue 7230, when multiple useOnWindow are used in a component that is not rendered, it should add multiple script nodes', async () => {
JerryWu1234 marked this conversation as resolved.
Show resolved Hide resolved
const BreakpointProvider = component$(() => {
useOnDocument(
'click',
$(() => {})
);

useOnWindow(
'resize',
$(() => {})
);

useVisibleTask$(() => {});

return <Slot />;
});

const LayoutTest = component$(() => {
return (
<BreakpointProvider>
<div>test</div>
</BreakpointProvider>
);
});
const { vNode } = await render(<LayoutTest />, { debug });
expect(vNode).toMatchVDOM(
<Component ssr-required>
<Component ssr-required>
<Component ssr-required>
<Component ssr-required>
<div>test</div>
</Component>
<script type="placeholder" hidden></script>
<script type="placeholder" hidden></script>
<script type="placeholder" hidden></script>
</Component>
</Component>
</Component>
);
});
it('issue 7230, when useOnDocument is used in a component that is not rendered, it should add a script node', async () => {
JerryWu1234 marked this conversation as resolved.
Show resolved Hide resolved
const BreakpointProvider = component$(() => {
useOnDocument(
'click',
$(() => {})
);

return <Slot />;
});

const Layout = component$(() => {
return (
<BreakpointProvider>
<div>test</div>
</BreakpointProvider>
);
});
const { vNode } = await render(<Layout />, { debug });
expect(vNode).toMatchVDOM(
<Component ssr-required>
<Component ssr-required>
<Component ssr-required>
<Projection ssr-required>
<div>test</div>
</Projection>
<script type="placeholder" hidden></script>
</Component>
</Component>
</Component>
);
});
});
Loading