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

Component that changes a sub-property of its own state object by async method re-renders the whole component in an infinite loop #98

Open
Clonkex opened this issue Feb 12, 2024 · 6 comments

Comments

@Clonkex
Copy link

Clonkex commented Feb 12, 2024

This one is quite confusing. I'm not sure if I'm doing something wrong or if it's a bug in ArrowJS.

This is as simple as I can make it while still replicating the behaviour:

https://jsfiddle.net/9d51qhe2

<script type="module">
    import { reactive, html } from 'https://esm.sh/@arrow-js/core';

    const main = getMain();
    main(document.querySelector('#container'));

    function getMain() {
        return html`
            <div>
                ${() => html`
                    ${() => console.log('zz0mkno')}
                    ${getLoginComponent()}
                `}
            </div>
        `;
    }

    function getLoginComponent() {
        const state = reactive({
            pageDatas: [],
        });
        setTimeout(async () => {
            state.pageDatas.push(Date.now());
        }, 500);

        return html`<div>bob</div>`;
    }
</script>

<div id="container">
</div>

If you run the fiddle, you'll see it continually prints zz0mkno. It's re-rendering the entire getLoginComponent component every time in an infinite loop.

Here's a fiddle that demonstrates more closely what I was doing when I came across this bug (including a network request instead of a timeout). Basically I was loading some user data on the login page to allow it to do client-side validation before making a server request to log in.

@Aidenir
Copy link

Aidenir commented Mar 12, 2024

I've run into the same issue just now. Did you ever found a solution/workaround @Clonkex ?

@Clonkex
Copy link
Author

Clonkex commented Mar 12, 2024

@Aidenir Unfortunately my "solution" was to switch to Preact + HTM (which is relatively easy to achieve even with an existing ArrowJS codebase because the ArrowJS and HTM syntax is similar - it's also still fully clientside, no build step necessary). It's not quite as elegant as ArrowJS in some ways (in particular, it took me a while to get my head around state handling - having to set an entirely new copy of the object when anything changes feels jank, but also potentially simpler and more reliable) but in other ways it's simpler and has the significant benefit of being battle-tested. If you make the same switch, just know that when you're trying to understand some Preact feature, search for React first because Preact is basically just a reimplementation of React.

So yeah, I was unable to work around this problem and it was enough to make me suffer the pain of learning Preact instead.

@Aidenir
Copy link

Aidenir commented Mar 12, 2024

Alright, thanks for the reply @Clonkex :) . I did find a super janky workaround I think, where I changed the type of the value I keep in the reactive state to a string instead of an array: https://jsfiddle.net/09Lwopqy/

    function getLoginComponent() {
        const state = reactive({
            pageDatas: "[]",
        });
        setTimeout(async () => {
        		state.pageDatas = JSON.stringify(JSON.parse(state.pageDatas).push(Date.now()))
        }, 500);

        return html`<div>bob</div>`;
    }

From limited testing this at least solved my issue for now, and while it's definitely not a good solution, it let me keep working on the other problems in my app until hopefully a better solution comes along :P

@jfftck
Copy link

jfftck commented Mar 27, 2024

Alright, thanks for the reply @Clonkex :) . I did find a super janky workaround I think, where I changed the type of the value I keep in the reactive state to a string instead of an array: https://jsfiddle.net/09Lwopqy/

    function getLoginComponent() {
        const state = reactive({
            pageDatas: "[]",
        });
        setTimeout(async () => {
        		state.pageDatas = JSON.stringify(JSON.parse(state.pageDatas).push(Date.now()))
        }, 500);

        return html`<div>bob</div>`;
    }

From limited testing this at least solved my issue for now, and while it's definitely not a good solution, it let me keep working on the other problems in my app until hopefully a better solution comes along :P

There are things wrong with this code and the code above it:

  1. setTimeout doesn't take awaitable functions, remove the async in front of the lambda
  2. You shouldn't set your reactive variable to an object that has one property pageDatas, just use the array directly and it should work fine

Try this: const pageTimestamp = reactive([])
Then you should be able to use it like this: pageTimestamp.push(Date.now())

Also, the naming of the variable to state doesn't describe what you are using it for and pageDatas is also poorly named as it looks to be timestamps. If you need to keep state then you would need to spread the array, as only the object changes are being tracked, so changing the child array directly will never trigger the change event.

Here is how keeping the object would work: state.pageTimestamps = [...state.pageTimestamps, Date.now()]

Please note that using the object will cause additional rerendering if any other properties are attached to that object. That is why I first recommended making the array reactive.

@Clonkex
Copy link
Author

Clonkex commented Mar 27, 2024

@jfftck You're reading too much into my testing code haha. It was cut down from real code and I didn't rename some things.

setTimeout doesn't take awaitable functions, remove the async in front of the lambda

Well in my original code the lambda awaited some stuff, which is why the async is there. I just forgot to remove it. It also makes no difference to setTimeout whether the function is awaitable or not; if the lambda awaits anything it needs to be marked as async.

You shouldn't set your reactive variable to an object that has one property pageDatas, just use the array directly and it should work fine

It's been a while, but I believe I tried this and it made no difference. Or maybe it did make a difference but I couldn't do that because my real state was more complex and had multiple fields.

@jfftck
Copy link

jfftck commented Mar 27, 2024

But your problem of circular loop can be fixed by splitting your state, anything that is for the component directly should be separate from the state for the children. Having it as 1 shared state can cause any changes to rerender both components.

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

No branches or pull requests

3 participants