[🐞] embedded QRL on non-$-terminated prop halts optimizer

bebraw commented 9 months ago

Which component is affected?

Qwik Runtime

Describe the bug

Instead of logging, console.log within $(...) at a prop callback gives an error (action works as expected, however).

Reproduction

https://github.com/bebraw/qwik-repro

Steps to reproduce

After npm install and npm run dev, you should see Optimizer should replace all usages of $() with some special syntax. If you need to create a QRL manually, use inlinedQrl() instead.. The file in question is /src/routes/index.tsx.

It's possible it's a user error but I believe in that case the error could be more instructive.

System Info

System:
    OS: macOS 14.0
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
    Memory: 11.11 GB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 16.15.0 - ~/.nvm/versions/node/v16.15.0/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 8.5.5 - ~/.nvm/versions/node/v16.15.0/bin/npm
    pnpm: 8.9.0 - /usr/local/bin/pnpm
    Watchman: 2023.10.09.00 - /usr/local/bin/watchman
  Browsers:
    Chrome: 117.0.5938.149
    Edge: 117.0.2045.55
    Safari: 17.0
  npmPackages:
    @builder.io/qwik: ^1.2.13 => 1.2.13
    @builder.io/qwik-city: ^1.2.13 => 1.2.13
    undici: 5.25.4 => 5.25.4
    vite: 4.4.11 => 4.4.11

Additional Information

No response

wmertens commented 9 months ago

Hi Juho, long time no speak! Good to see you here.

The user error is that you should rename onItemsChanged to onItemsChanged$, and then you can even leave off the $(). The optimizer will automatically convert the callback into a QRL, and then in your component you can call it as normal, but it will return a Promise.

But there is an optimizer bug here as well. See this playground, if you move the hmm into the foo={hmm}, it breaks the build.

console.log is not needed here, even $(() => {}) will break it.

bebraw commented 9 months ago

Nice to see you too.

Thanks for the solution. I somehow missed that convention. The most curious thing is that it works when the handler is bound to an action and it's only with console.log where it crashes but now I know better.

It's still worth fixing the compiler bug, yeah.

thejackshelton commented 9 months ago

Do we think this is still an issue? Or is this something we can close? @wmertens @bebraw

bebraw commented 9 months ago

@thejackshelton It's still visible, yes. I updated my demonstration repository to match the latest versions.

wmertens commented 6 months ago

So the problem is that the optimizer converts

 <Cmp foo={$(() => console.log('hi there'))}>Hello Qwik</Cmp>;

into

    return /*#__PURE__*/ _jsxC(Cmp, {
        get foo () {
            return $(()=>console.log('hi there'));
        },
        children: "Hello Qwik",
        [_IMMUTABLE]: {
            foo: _IMMUTABLE
        }
    }, 3, "4e_1", {
        fileName: "app.tsx",
        lineNumber: 9,
        columnNumber: 10
    });

and doesn't replace the $()

QwikDev / qwik