QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.86k stars 1.31k forks source link

[🐞] diffChildren can hit undefined on investigated children #5476

Open eglove opened 1 year ago

eglove commented 1 year ago

Which component is affected?

Qwik City (routing)

Describe the bug

I have multiple loaders on the same route. Whether I should be using multiple loaders on the same route is beside the point. But it's explained as possible here. However, I get inconsistent behavior with them.

I want to be clear. The fact that this is happening has nothing to do with me personally or any maintainers. We are looking at code and observing a framework in its infancy. The issue here is about USE and RESULTS. Not about the functionality of a repro and whether you want to do something differently.

I have two loaders getting results from two paginations.

Action 1: Click next on ONLY the post of the pagination. This will use <Link /> to route to /loader-action-sync?posts=2. Then click on the top link which uses <Link /> to route to /loader-action-sync. The routeLoader will use the default page 1, and posts on the UI will return to page 1 as expected.

Action 2: Click next once on BOTH the post and comment pagination. This will put you on /loader-action-sync?posts=2&comments=2. Then click on that top link to go back to /loader-action-sync. The routeLoader will get and return data for page 1 on each. But the UI will not update.

If you just click around on the paginations in general, you'll see a lot of sync issues. Sometimes nothing happens at all, for example. Sometimes the loader runs but the URL doesn't change.

Reproduction

https://github.com/eglove/examples/blob/main/apps/qwik-example/src/routes/loader-action-sync/index.tsx

Steps to reproduce

  1. git clone https://github.com/eglove/examples
  2. cd apps/qwik-example
  3. pnpm i && pnpm dev
  4. Go to http://localhost:5173/loader-action-sync

System Info

System:
    OS: Windows 11 10.0.22621
    CPU: (24) x64 12th Gen Intel(R) Core(TM) i9-12900K
    Memory: 11.06 GB / 31.76 GB
  Binaries:
    Node: 20.10.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.21 - C:\Program Files\nodejs\yarn.CMD
    npm: 10.2.4 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.10.5 - C:\Program Files\nodejs\pnpm.CMD
  Browsers:
    Edge: Chromium (119.0.2151.58)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    @builder.io/qwik: ^1.2.19 => 1.2.19
    @builder.io/qwik-city: ^1.2.19 => 1.2.19
    undici: ^5.26.0 => 5.27.2
    vite: ^4.4.11 => 4.5.0

Additional Information

No response

eglove commented 1 year ago

I found a very specific fix for this.

First Links need to have replaceState. This is fine, but what people are calling "MPA" and "SPA" modes is what causes the inconsistency here. And we can't expect users to disable javascript in order for these apps to function consistently. Client side navigation will struggle to get default states back. This is the same issue in the input inconsistencies.

Second, and more oddly, a very specific useTask$ is required. Here, it's fine as it is. But on another project I had this:

useTask$(({ track }) => {
  currentPage.value = track(() => {
    return Number(location.url.searchParams.get(category) ?? '1');
  });
});

This did not work. But if I change it to this, it seems consistent so far.

    useTask$(({ track }) => {
      track(() => {
        return location.url.toString();
      });

      currentPage.value = Number(
        location.url.searchParams.get(category) ?? '1',
      );
    });

If I just set location.url... to a const, of course that doesn't work. I have to use a task to get this. Even though that doesn't make sense for the initial render. Many of these API's feel bipolar and unstable, I never know which version I'm working with or what magic incantation to speak with the same API's for different scenarios.

wmertens commented 1 year ago

Everything worked fine for me and then I noticed your repro has your proposed fix ;)

Anyway the reason it didn't work at first was because you triggered some bug. If you go back one commit and do your repro steps, you'll see two errors in the console (always pay attention to the console). The errors are in qwik core so that's why it didn't update properly.

The failure is in https://github.com/BuilderIO/qwik/blob/0e413eb78a965851cbc6a31fdd74a37e32fec091/packages/qwik/src/core/render/dom/visitor.ts#L216, at that time the oldCh array has 11 items, with 2 of them being undefined.

So you definitely hit a bug.

Other than that, I would write your currentpage as

const currentPage = useComputed$(() => Number(location.url.searchParams.get(category)) || 1)

Your current version gives you page 0 and is not resilient to invalid input data giving you NaN.

(Maybe even throw in a Math.min(input||0, 1))

I'll mark this as a bug.

wmertens commented 1 year ago

So what happens is that it's diffing the old vdom with the new, and for some reason the old vdom has undefined children, for the first two children of the "Comments" section.

I don't know this code so I don't know if children are allowed to be undefined. I can't spend more time on this right now, but to repro you need to be on commit 136863e and follow the OP instructions.

eglove commented 1 year ago

It's pulling from static JSON files copy/pasted from typicode, so there's never a case where anything is undefined.

I think what's strange is, I didn't make any commits to fix this, I see what you mean with the start variable being NaN, but that's not what I submitted the issue on, yet it seems ok for now. Hence the focus on consistency.

It's very difficult to reproduce bugs with QwikCity because behaviors are never the same. The last time I saw something so unpredictable was a 20-year-old PHP app in which class behavior changed if you put them in the wrong directory.

On my other project, useComputed makes no difference. No, there are no console logs, and no NaNs. I even use my own util library to get rid of this overhead. In this case, the UI doesn't update at all. It's essentially the same code So I'm not going to spend much more time on Qwik. I don't think it's ready for real projects, certainly nothing that needs to worry about scaling TBT. But I hope you guys can get enough funding to put some serious work into it. Best of luck.

wmertens commented 12 months ago

I didn't mean that you console logged anything. I meant that for some reason the sequence of things you do causes the vDOM to have 2 undefined children, even though they exist in the real DOM. It's not something you did in your code, it's a bug in Qwik core. It causes two uncaught exceptions to be thrown and logged, and further processing to halt.

I'm also not experiencing the inconsistencies in qwik-city you're seeing, maybe they stem from you hitting this bug? In any case thanks for reporting.

JerryWu1234 commented 2 months ago

Will his fix in V2? I can take a look if not