lostpebble / pullstate

Simple state stores using immer and React hooks - re-use parts of your state by pulling it anywhere you like!
https://lostpebble.github.io/pullstate
MIT License
1.08k stars 23 forks source link

Pullstate crash when upating object in array within async action #80

Closed xaralis closed 3 years ago

xaralis commented 3 years ago

I have following async action that should update one item's content in the postActionHook (I've left out the HTTP part for better clarity, findIndex comes from lodash and it seems to work fine - returns right index):

/**
 * Update content of an announcement.
 */
export const updateAnnouncementContent = createAsyncAction(
  /**
   *
   * @param {CF2021.Announcement} item
   */
  async (item, newContent) => {
    return successResult({ item, newContent });
  },
  {
    postActionHook: ({ result }) => {
      AnnouncementStore.update((state) => {
        const itemIdx = findIndex(state.items, { id: result.payload.item.id });
        state.items[itemIdx].content = result.payload.newContent;
      });
    },
  }
);

I'm using it in my component like this:

const confirmEdit = useCallback(
    async (newContent) => {
      await updateAnnouncementContent.run(itemToEdit, newContent);
      setItemToEdit(null);
    },
    [itemToEdit, setItemToEdit]
  );

Unfortunately, this ends up in crash:

image

I wasn't able to find much about createPullstateCore but only seems useful for updates that span multiple stores which is not the case. Also, doing other updates in similar manner like appending new items or removing items works just fine.

@lostpebble Can you please shed some light on this?

lostpebble commented 3 years ago

Hi @xaralis ,

Can I get a bit more context please:

  1. Where is this action running, server or client?
  2. Where are you accessing AnnouncementStore from?

Hopefully will help to get to the bottom of this, as this error should not appear unless you are using the direct stores which are passed to the post action hook- which it seems you are not doing.

xaralis commented 3 years ago

@lostpebble Thanks for quick reply! It is running on the client. AnnoucementStore is being accessed from the async action's postActionHook and from just a single React component ATM.

lostpebble commented 3 years ago

I mean where is AnnouncementStore coming from, as in where is it being imported? Just directly in the file, like

import { AnnoucementStore } from "./stores/Stores"

Or is it passed from somewhere else.

If its just imported directly, then its very strange that this proxy store error is occurring because it should never be referenced at all.

xaralis commented 3 years ago

Nope, it's imported from the main stores file:

import { AnnouncementStore } from "stores";

Over there, it's defined along bunch of others stores like this:

import { Store } from "pullstate";

... other stores code

/** @type {CF2021.AnnouncementStorePayload} */
const announcementStoreInitial = {
  items: [
    {
      id: "1",
      content:
        "Shizz fo shizzle mah nizzle fo rizzle, mah home g-dizzle, gravida vizzle, arcu. Pellentesque crunk tortizzle. Sed erizzle. Black izzle sheezy telliv.",
      datetime: new Date(),
      seen: false,
      type: "rejected-procedure-proposal",
    },
    {
      id: "2",
      content:
        "Shizz fo shizzle mah nizzle fo rizzle, mah home g-dizzle, gravida vizzle, arcu. Pellentesque crunk tortizzle. Sed erizzle. Black izzle sheezy telliv.",
      datetime: new Date(),
      seen: false,
      type: "accepted-procedure-proposal",
    },
    {
      id: "3",
      content:
        "Shizz fo shizzle mah nizzle fo rizzle, mah home g-dizzle, gravida vizzle, arcu. Pellentesque crunk tortizzle. Sed erizzle. Black izzle sheezy telliv.",
      datetime: new Date(),
      seen: true,
      type: "suggested-procedure-proposal",
    },
    {
      id: "4",
      content:
        "Shizz fo shizzle mah nizzle fo rizzle, mah home g-dizzle, gravida vizzle, arcu. Pellentesque crunk tortizzle. Sed erizzle. Black izzle sheezy telliv.",
      datetime: new Date(),
      seen: true,
      type: "voting",
    },
    {
      id: "5",
      content:
        "Shizz fo shizzle mah nizzle fo rizzle, mah home g-dizzle, gravida vizzle, arcu. Pellentesque crunk tortizzle. Sed erizzle. Black izzle sheezy telliv.",
      datetime: new Date(),
      seen: true,
      type: "announcement",
    },
    {
      id: "6",
      content:
        "Shizz fo shizzle mah nizzle fo rizzle, mah home g-dizzle, gravida vizzle, arcu. Pellentesque crunk tortizzle. Sed erizzle. Black izzle sheezy telliv.",
      datetime: new Date(),
      seen: true,
      type: "user-ban",
    },
  ],
};

export const AnnouncementStore = new Store(announcementStoreInitial);

... other stores code
lostpebble commented 3 years ago

Okay then its very strange. Somehow one of the proxy error systems is "hijacking" your store update when it reaches a certain point.

Will investigate further, thanks for bringing it up!

lostpebble commented 3 years ago

I'm struggling to create a reproduction of this error on my side. I've created a store and and an async action like yours- but it seems to work just fine.

Perhaps some recent changes I made (I'm using the master version) have fixed what could be your issue.

I'll push an update now, which you can try out.

lostpebble commented 3 years ago

Please try out 1.20.5 and see if its giving the same issues.

xaralis commented 3 years ago

I'm sorry but it didn't help. What immer version are you using? I'm at 7.0.15.

lostpebble commented 3 years ago

I'm currently on 7.0.9 - going to try update and see.

lostpebble commented 3 years ago

Hmm, I updated to 7.0.15 and its still no issue from my tests.

If you could somehow create a small repro it would be very helpful, as I'll never be able to exactly replicate your situation.

You can start with this one as a base (the basic pullstate example): https://codesandbox.io/s/myvj8zzypp

Either that or a small GitHub repo with the issue showing.

xaralis commented 3 years ago

The app repository is public:

https://gitlab.pirati.cz/pak/cf2021/-/tree/master/

Feel free to clone it, then just:

npm install
REACT_APP_STYLEGUIDE_URL=https://styleguide.pir-test.eu/latest/ npm start

The error will trigger when confirming the Edit action - pencil icon with "Upravit" title:

image

A modal will show up, then just confirm by clicking on "Uložit".

image

The action is implemented here: https://gitlab.pirati.cz/pak/cf2021/-/blob/master/src/actions/announcements.js#L59 The component is implemented here: https://gitlab.pirati.cz/pak/cf2021/-/blob/master/src/containers/AnnoucementsContainer.jsx#L17 Store is defined here: https://gitlab.pirati.cz/pak/cf2021/-/blob/master/src/stores.js#L78

Thanks for investigating!

lostpebble commented 3 years ago

Okay I've managed to look deeper and noticed that you are actually not using the Async Actions correctly.

Async Actions only take a single argument for inputs, an object inside which you need to pass all arguments needed for this action, so this is incorrect:

await updateAnnouncementContent.run(itemToEdit, newContent);

And this:

async (item, newContent) => { /* action */ }

It should be:

await updateAnnouncementContent.run({ item: itemToEdit, newContent });

and

async ({ item, newContent }) => {
    return successResult({ item, newContent });
}

Respectively.

If you were using TypeScript, this would have been made more obvious because errors would pop up and show you the second argument is for run options.

When these changes are made, then the code starts working again. I think something you were passing in newContent as "options" was causing the errors.

Please try these changes and let me know if you come right.

xaralis commented 3 years ago

Awesome! Thanks and sorry for taking your time!