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

[🐞]Store array update does not rerender list #5662

Closed
Domniq opened this issue Jan 2, 2024 · 28 comments
Closed

[🐞]Store array update does not rerender list #5662

Domniq opened this issue Jan 2, 2024 · 28 comments

Comments

@Domniq
Copy link

Domniq commented Jan 2, 2024

Which component is affected?

Qwik Runtime

Describe the bug

There is a problem with rendering updated values using list { store.array.map }
I can change values only one or twice, depending on code usage after that any further modification is not updating values visually, however store values are changed
I've created thread on discord https://discord.com/channels/842438759945601056/1191080926554361992
I managed to find a solution and it's working but it should be used this way

As you can see on the image store.values are different than those that are visible using .map
problem

As i mentioned earlier 1 or 2 operations are displayed any further only updates store values but not items rendered via { store.array.map }

IMPORTANT:
This code is working in stackblitz https://stackblitz.com/edit/qwik-starter-kzdpd9 (version 1.2.11)

Reproduction

https://qwik.builder.io/playground/#v=1.3.1&f=7VjdbtMwFH4VSxNyAnVI0r80I5UGmgSIAdLgapqQUztrWLdEo6Xrqt7s3fZenGMnTtqU%2FYB2MWmbFDmxfc7xd%2F4%2BtxY0ftCteUp5RjfdVi0YWlUv1kPV3O8RV0reV2id7yBqTyDNoW1xgATBSEVY8IMWvGHlKt8B8nsFZsEklDFRxRGWkLmoDMpI2FB%2BdLwaIv64HyTUVh4VoC2VZdQPuOvLfpe1AzdgHd7rMt5ORmzQd92BL%2BRADHzaKuym309vrrkg84WQo3E2X1CywlM9TJ5Xyds7nZ3NJlhotojiXhz3ZdBmXLY56%2FQ9ybgbJ2zkcVAi3F7P6zZMu%2BL5zXUGjGGLwH7cSdpeIFgwij3WEd2EDQYJZ57vxnGvG7STON4icA64XcnzlBuJx%2FBUwyV4TuahboRYJoy7UlH4SlM77fGhRaletC1FkIyWFI0Cm8mhBYUkmcjLXXLCQYvn5jD8OYNenywYNiaIFIimHDgQi%2BV0LuX5LhCkSzZPxXQckq6LO%2BjKJMotGvDJRHqhGzVQ32wyOwNxSnMHFedcCDhESHxlB7QjyAawKr8k0MXhwDuijf%2FlFLvgIp1ByKndFY8ByycZbNxJkmQX6IUy1XPdFzVLsVz4wz0hkO5AKfBrE6kiwJhzEcX%2BQAHsiIJxP9BzlMCpRnKcYa5GtEwKaqoFFhPF8ityu9TJBuoslWb2ClWb24ApM3hLeKJQ7ot0ei8cJSz8XyAP%2BW%2BpkWyBTEATP9wCZzE02aq2OlXVcuDSYVnF%2B6KlWJ1SpPPH%2BONULqKlml09biIpjfmwLMELJxUr5Apr001Y1L2iOofGphKCqK%2BLWQs524aGUXxX90p9RNq0c7j8ePjls6OLDhyxjOo3r3HfZqPEimR4hMmBkNzVYDS90OVOMfIIGLQmfw5w7f2JxOHbxQdhVdlpA18m778dfFLX2GKNKolpQixc4gD5nMHFVjP7IXG1nlJTiVbTJBKpZXqxbrzWAZ%2BOnWSSZRd6iFef7Azc8ZJYkCfqjzBMGRy8Ip5t41O92s60ZEt22Ul0b6jMVJ8LtzSiNp%2F9GhtvA8qlAL0V4KLAIXA7CDAO2AiS5jExbMKCvZR95SGuqBL8L77YMHItPLWnRHMyFUgha8cwJeDuQHr0Ez1SdJU4%2FFNwwE8R%2B3w0hrIGxzcFzYgHc0cAKomiNZBtUxo2BR4pIcc1n%2BiAK1jJ1shbc6UJxuerztO76vwB

Steps to reproduce

Here are different ways to modify store.arrays:
// Not working #1 (can modify 1 element)
const category: PartCategoryData =
{
id: id.value,
name: name.value
}

store.categories.forEach((c, index) => {
  if (c.id === category.id)
    store.categories[index] = category;
});


// Not working #2 (can modify 1 element)
store.categories = store.categories.map((category) => {
  if (category.id === id.value) {
    return {
      ...category, name: name.value
    };
  }

  return category;
});


// Not working #3 (can modify 1 element)
const modified = store.categories.map((category) => {
  if (category.id === id.value) {
    return {
      ...category, name: name.value
    };
  }

  return category;
});

store.categories.splice(0, store.categories.length, ...modified);


// Not working #4 (can modify 2 elements)
const selectedCategory = store.categories.find(category => category.id === id.value)

if (selectedCategory) {
  selectedCategory.name = name.value;
}


// Not working #5 (can modify 2 elements)
store.categories.forEach(category => {
  if (category.id === id.value) {
    category.name = name.value;
  }
});


// Not working #6 (can modify 2 elements)
for (const category of store.categories) {
  if (category.id === id.value) {
    category.name = name.value;
    break; // If you only want to modify the first matching item
  }
}

// Not working #7 (can modify 1 element)
store.categories = store.categories
.filter(category => !(category.id === id.value))
.concat(store.categories.filter(category => category.id === id.value)
  .map(category => ({ ...category, name: name.value }))
);

System Info

Windows 11, Opera browser, mozilla, edge
In my project im using Qwik ^1.2.15

Additional Information

You can find more on discord thread https://discord.com/channels/842438759945601056/1191080926554361992

@Domniq Domniq added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels Jan 2, 2024
@wmertens
Copy link
Member

wmertens commented Jan 2, 2024

Did Qwik 1.3.2 fix anything?

@Domniq
Copy link
Author

Domniq commented Jan 2, 2024

Nah, still an issue
image

I managed to find workaround by clearing store and assigning it once again after 'empty' await
image

Without await Promise it does not update displayed items, only store gets updated
This version if working fully https://stackblitz.com/edit/qwik-starter-kzdpd9

@wmertens
Copy link
Member

wmertens commented Jan 2, 2024

It seems that 1.2.13 works correct, right?

@Domniq
Copy link
Author

Domniq commented Jan 2, 2024

Stackblitz link I've provided uses
"@builder.io/qwik": "^1.2.11",
"@builder.io/qwik-city": "^1.2.11",
It working fine there

@Domniq
Copy link
Author

Domniq commented Jan 2, 2024

I did some test on qwik@1.2.11 and it's not working there.
So only in Stackblitz with qwik 1.2.11 it's working, might it be environment issue?

@wmertens
Copy link
Member

wmertens commented Jan 2, 2024

Very odd. Ideally we'd have a minimal e2e test case but if it even differs across environments...

@Domniq
Copy link
Author

Domniq commented Jan 2, 2024

My bad, i've provided wrong link for stackblitz, here is correct https://stackblitz.com/edit/qwik-starter-kzdpd9, previous version was modified.
I downloaded this stackblitz repo and run it on localhost, now it is working on 1.2.11

@wmertens
Copy link
Member

wmertens commented Jan 2, 2024

Ok in that case the playground is equally valid.

@Domniq
Copy link
Author

Domniq commented Jan 2, 2024

Yes, currently im using Code Compare for file difference with node_modules because everything matches except node_modules folder
npm create qwik@1.2.11 and downloaded stackblitz project are compared

@Domniq
Copy link
Author

Domniq commented Jan 2, 2024

package-lock.json differs https://www.diffchecker.com/TwlJfa9s/
On the left npm create qwik@1.2.11, on the right taken from stackblitz

@wmertens
Copy link
Member

Can you make a minimal playground repro that always triggers the bug? I'm having trouble reproducing it.

@wmertens wmertens added COMP: runtime COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it P2: important and removed STATUS-1: needs triage New issue which needs to be triaged labels Jan 17, 2024
@Domniq
Copy link
Author

Domniq commented Jan 18, 2024

Here is minimalistic version with several solutions to save updated values in store
Quick descripiton:

  • you can add new entry in 'Add new' form
  • when you click on green label you can edit entry in 'Edit' form
  • when you click on green you can remove entry

I've tested it few time on 1.3.5 qwik.builder.io/playground
removing - working
adding - working
editing - works only for 1 / 2 operations of changing value of element (value in store.categories is updated, but rendered items via object.map isn't)

In my project I have found that sometimes wrong index / key is removed

Here is link for code: https://pastebin.com/4MiRegHd
Or download zip with index.tsx from attachments
index.zip

@the-zimmermann
Copy link
Contributor

@Domniq isn't the deep option on useStore change this behavior?

@wmertens
Copy link
Member

@Domniq can I ask you to make the repro smaller, in the playground?

Ideally, you make it so that only a few clicks are needed to trigger the problem, and the app should be as small as possible, no styling or nice content. Just exactly what is needed to trigger.

Here's the playground link to start from: link

@Domniq
Copy link
Author

Domniq commented Jan 20, 2024

@the-zimmermann it does not change anything

Here is smaller repo
https://qwik.builder.io/playground/#v=1.4.0&f=7VfdbtMwFL7nKbwrJ9BESbv%2BpUsQg0kMMUCUu2lCTu2sYd1Stenaquq7c44dx0nLVqZpF0jsoosT%2B%2Fx%2B55zPFdA0%2B9VMycyooduogKFhZrF6xFksn2S%2FewxhRIdEiv4GU%2FQ9APgaKh4mGIPoYFxSHhRUoQErbGJ6DdEvJTwK0oJVSNYRGr6wgSpGbdBSgj3tl1fbCHOB50FCZedlEcCNNI02e8xrim7bafW8nnPMOm2HtZKR0%2B96Xr%2FJRZ%2F3m7RRGE7PVmO2mOeUbNGbp4nxjZhTlsMYX%2B%2BLYX4cd0Wv5TDRYs5x1xcO8%2BLEGfkMFHCv0%2FHbRsz59V2KoN%2BX042Pk5bf405vFPvOMW8nTr%2BfMMdvenHcafdaSRwbOcPFfCru5lVJV%2FC7bZANF2IayJnXAIgzqLt7OIFrbBCkzE7Ki9QoVqcyHFmUFrseKA%2BgoJqgUeAyUxhAAUkmYjUgvyDOabJ2cBYBIAA1U6A9TizypRB3A%2BBEK2eZ8nwckLbnTVcDuq3UCnLbcgXrVBJIBGpIsb9SsDikjPOfGANKQPNIjDNEeEg1kGhZbaocJU829HCjIPqOc0uC095G8FzyaWOMZNpPtU3wNH%2B%2BcUN2L5R1DZAKFuKLAybqRQkped41ReQCH7asYr1GPTXnyI1Yh7o6127Kty%2BSY1A2LQTTUTbJZgGgjA%2FIaDGb40LSKjEb0L2gfBe3mQlLxVCIT81uHIWP6bueSTMPaxxKdl2GrMiF0YV5rmurZcTGVqltiE4VRwWKBfW4AKJ5EkfSF8hrBNSMfBp%2B%2FeKqmQfRLcBZkX6CN7RoI7epSjXbwAr5dWewyDIuB%2B%2BOO%2FvtFx0Mimmjm4GazKpdSDIbAvlUvMkFmno2Efh4uj7nVgX8Njr08cfFZ3kFLDaBMVKEC5wN2HhocohvVcdJ%2Bf7XlOPJ6jVS75JkoOphWTcBOTRoXt5ZdCchlnHZLe4AEfGUJq1rtVqBqt2CRa8JgYsgsdQ2HRGSJXhEy1BqKuEiYRiWkTS7yomqAg4ajWlKV22PHA5aivkew0C50UuIvvqV%2F3cdgPNgJu7dQp5eqetHmaxqPf9tulSelEvo9NFSknYX4pOks1uLfsjIOluQJYPbY55Ba0EdcNmDssML11tq29JUNdlUkv5g9l7vTNIJ9Ih6%2B6yF6kiGfBeQ5ZQ57OBTsFdOwOdD7%2BFuQEK5rcQYhN%2B6YPnYTSZZNlOPeD%2FPbqFbviaW76k%2F4hD1SN4Q37bxVy5tN9eU3tbER1EZY%2BYhOF26LvS%2B%2BmszC64ULGs9hiLllfgj%2Fy9G%2F97F6Dc

Here is video for you to see what's wrong
https://www.youtube.com/watch?v=curY0UAvcLg

@gioboa
Copy link
Member

gioboa commented Mar 5, 2024

It's really strange because I can't reproduce the problem constantly. I'll add a specific test in the Qwik v2 test just in case.

@gioboa gioboa added VERSION: upcoming major and removed COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it labels Mar 5, 2024
@gioboa
Copy link
Member

gioboa commented Mar 5, 2024

This example always fails 👀

import { component$, useStore } from '@builder.io/qwik';

export default component$(() => {
  const store = useStore<{ users: { name: string }[] }>(
    { users: [{ name: 'Misko' }] },
    { deep: true, reactive: true }
  );

  return (
    <>
      {store.users.map((user, key) => (
        <div key={key}>
          <div
            onClick$={() => {
              store.users = store.users.map(({ name }: { name: string }) => ({
                name: name === user.name ? name + '!' : name,
              }));
            }}
          >
            {user.name}
          </div>
        </div>
      ))}
      <span>{JSON.stringify(store)}</span>
    </>
  );
});

gioboa added a commit to gioboa/qwik that referenced this issue Mar 5, 2024
@gioboa
Copy link
Member

gioboa commented Mar 5, 2024

You found a workaround and that's okayish but with Qwik v2 it's working fine.
@Domniq thanks for opening this issue because it helps us to improve the codebase.

gioboa added a commit that referenced this issue Mar 5, 2024
@Domniq
Copy link
Author

Domniq commented Mar 6, 2024

You found a workaround and that's okayish but with Qwik v2 it's working fine. @Domniq thanks for opening this issue because it helps us to improve the codebase.

In most cases that workaround does the job well, sometimes it does not and i have to press button twice in order to "refresh" (switch tabs that filters "store" by status (int), stored array Type has 7 fields with various types)

Qwik v2 - you mean upcoming major update?

@gioboa
Copy link
Member

gioboa commented Mar 6, 2024

Yep, the new version of the rendering engine. This change will not affect final users, you can check the build/v2 branch if you are curious.

@Domniq
Copy link
Author

Domniq commented Mar 6, 2024

Great news! :)

@mhevery
Copy link
Contributor

mhevery commented Mar 27, 2024

ROOT CAUSE ANALYSIS: This is a bug in Optimizer. The optimizer incorrectly marks the onClick listener as 'const'/'immutable'. Because it is const, the QRL associated with the click handler always points to the original object, and it is not updated.

@wmertens
Copy link
Member

@mhevery so this is also a problem in V2?

@jahwin
Copy link

jahwin commented Jul 30, 2024

Hey @mhevery and @gioboa,

I encountered the same issue. When running this code:

import { component$, useStore } from '@builder.io/qwik';

export default component$(() => {
  const tasks = useStore<{ label: string; prio: number }[]>([]);

  return (
    <>
      <div>
        <p>
          <button
            onClick$={() => {
              tasks.push({ label: 'New Task' + tasks.length, prio: 1 });
            }}
          >
            Add new Task
          </button>
        </p>
        <ul>
          {tasks.map((task, index) => (
            <li key={index} class="flex">
              <span>{task.label}</span>
              <span>=====</span>
              <button
                onClick$={() => {
                  tasks.splice(index, 1);
                }}
              >
                Remove
              </button>
            </li>
          ))}
        </ul>
      </div>
    </>
  );
});

the removal of elements is not working.

I tested this on versions from 1.0.0 to 1.2.14, and it worked fine. However, from version 1.3.0 to 1.7.2, it's not working.

@gioboa
Copy link
Member

gioboa commented Jul 30, 2024

I see, thanks for your detailed information.
@Varixo do we have a test case for this in v2?

@wmertens
Copy link
Member

It doesn't seem to be the optimizer, the code and html between versions remains the same. Something in 1.3 incorrectly removes nodes.

Here's an illustration of the bug:

  • add a few tasks
  • remove one in the middle
  • notice that the one at the end seems to have been removed
  • click the rerender button
  • notice that the one in the middle was actually removed

@jahwin
Copy link

jahwin commented Aug 1, 2024

I have continued testing and would like to update you on my findings. In a previous message, I mentioned that versions from 1.0.0 to 1.2.14 worked fine, but versions 1.3.0 to 1.7.2 did not. However, I have discovered that even in versions 1.0.0 to 1.2.14, including a component reintroduces the issue.

Here are the steps to reproduce the problem:

  1. Add a few tasks.
  2. Remove one task in the middle.
  3. Notice that the task at the end appears to have been removed.
  4. Click the rerender button.
  5. Observe that the task in the middle was actually removed.

Below is the relevant code snippet:

import { component$, useStore, useSignal } from '@builder.io/qwik';

export const TaskItem = component$((props: { name: string }) => {
  return <p>Hello {props.name}</p>;
});

export const Counter = component$(() => {
  const rerender = useSignal(0);
  const tasks = useStore<{ label: string; key: number }[]>([]);

  return (
    <>
      <div key={rerender.value}>
        <p>
          <button
            onClick$={() => {
              tasks.push({ label: 'New Task' + tasks.length, key: 1 });
            }}
          >
            Add new Task
          </button>
          <button
            onClick$={() => rerender.value++}
          >
            Rerender
          </button>
        </p>
        <ul>
          {tasks.map((task, index) => (
            <li key={task.key} class="flex">
            <TaskItem name={task.label} /> 
             {/* <span>{task.label}</span> */}
              <span>&nbsp;</span>
              <button
                onClick$={() => {
                  tasks.splice(index, 1);
                }}
              >
                Remove
              </button>
            </li>
          ))}
        </ul>
      </div>
    </>
  );
});

@mhevery , @wmertens , @gioboa , could you please address this issue in the next release (version 1.7.4)? We have two significant projects built using Qwik, and switching frameworks is not a feasible option for us. Your prompt attention to this matter would be greatly appreciated. Thank you.

@wmertens
Copy link
Member

wmertens commented Nov 1, 2024

This is all fixed in v2, including the shorter repro I linked here #5662 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

7 participants