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

setState() callback not invoked (since 8.3.0) #1840

Closed
mindplay-dk opened this issue Aug 5, 2019 · 2 comments · Fixed by #1857
Closed

setState() callback not invoked (since 8.3.0) #1840

mindplay-dk opened this issue Aug 5, 2019 · 2 comments · Fixed by #1857

Comments

@mindplay-dk
Copy link

I'm upgrading a project from preact^8.2 to preact^10.0.0-rc1 and ran into a problem.

From my component's constructor, I'm invoking a private method that calls setState() after initially loading some search results. This setState() call needs to update a private field after the update has finished, so I'm using the setState() callback.

Up to and including release 8.2.9, this worked fine.

Starting with release 8.3.0, the callback is no longer being called.

Current latest release 10.0.0-rc.1 is also affected.

Here's a repro:

https://codesandbox.io/s/silly-bhabha-x2roy

You should see wut? in the console, and the text should change to loaded, but never does.

Granted, calls to setState() don't work at all in React, so it's not an incompatibility with React - but it is a breaking change from previous versions of Preact, and it was tagged as a feature release, so in semver terms, this is either versioned incorrectly, or it's simply a regression/bug.

If this is difficult to fix for some reason, I'd suggest a proper error message, e.g. "setState() cannot be called from the constructor". (ReactDOM gives an error message along those lines. I know you guys are counting bytes - but isolating a silent error like this can take a while.)

Known work-around: defer the setState() call using e.g.:

setTimeout(() => this.setState({...}), 0);

Could be related to #556 or #1741. (If those are related, and if this issue isn't going to be addressed because of size concerns, it might make sense to close all of these issues together - perhaps adding an error message or documenting this pitfall.)

@marvinhagemeister
Copy link
Member

Calling setState inside the constructor is essentially a no-op. These calls should be moved to componentDidMount to work. In my experience it's always a good idea not to trigger any sort of effects when constructing things. It's usually confusing for co-workers and can have a negative effect on performance (mainly because it's unexpected behavior).

Good idea! We should add an error message for that 👍

(For reference: react does nothing there too https://codesandbox.io/s/react-setstate-constructor-q05j4)

@mindplay-dk
Copy link
Author

We should add an error message for that 👍

This will cost you precious, unrecoverable bytes, Marvin! 😜

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

Successfully merging a pull request may close this issue.

2 participants