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

Something got broken in 1.8.1 or 1.8.0 #79

Closed
mindplay-dk opened this issue Oct 16, 2019 · 15 comments · Fixed by #81
Closed

Something got broken in 1.8.1 or 1.8.0 #79

mindplay-dk opened this issue Oct 16, 2019 · 15 comments · Fixed by #81

Comments

@mindplay-dk
Copy link
Contributor

Here's a little experiment I've been working on:

https://codesandbox.io/s/fre-demo-iefch

I wanted to see if it was possible to implement React's class-based Component API using hooks.

This was working fine until release 1.8.1 - so the last release that worked for me was 1.8.0.

Try changing the dependency version from 1.8.0 to 1.8.1 in the sandbox, and all the calls to setState for some reason stops working. 😶

I suspected this had something to do with my recent change inlining the setter-function - but I've tried reverting that change, and it still doesn't work, so that's not why.

Here's a diff between 1.8.0 and 1.8.1.

Can you spot the problem in these changes?

Whatever it is, it's obviously not covered by tests yet...

@mindplay-dk mindplay-dk changed the title Something got broken in 1.8.1 Something got broken in 1.8.1 or 1.8.0 Oct 16, 2019
@yisar
Copy link
Collaborator

yisar commented Oct 16, 2019

How about 1.8.3

@mindplay-dk
Copy link
Contributor Author

How about 1.8.3

All versions after 1.8.1 affect that example.

Here's another simpler example:

https://codesandbox.io/s/fre-demo-uxuic

This one starts to break with version 1.8.0 already, so the bug may have been partially introduced there.

Watch:

bug

All of a sudden, it's like the scheduler stops accepting new tasks or something?

If you look in the console, you can see event handlers are still firing - it's the calls to the useState setters that aren't triggering updates.

Could be related to this maybe?

@yisar
Copy link
Collaborator

yisar commented Oct 16, 2019

This looks like 1.8.0 is wrong, in version 1.8.3 I removed [] and it worked

const component = useMemo(() => new Component(props));

@mindplay-dk
Copy link
Contributor Author

Okay, we still have missing tests for useEffect with different deps arguments - all the tests use an array right now. If this is the problem, I guess that's why the tests didn't catch it.

That might explain the problem with the first example.

But according to useMemo docs:

If no array is provided, a new value will be computed on every render.

That's not what I want in the first example - I'm passing [] because the function has no dependencies and should be executed only on the first render.

And the second example? The error is periodic, as you can see in the GIF - so probably has something to do with the scheduler? Might be two separate problems.

Gotta get the rest of the tests in place...

@yisar
Copy link
Collaborator

yisar commented Oct 16, 2019

I'm sorry, your code is correct, I just debugged it, this is indeed my fault, I will fix it as soon as possible, thanks!

@mindplay-dk
Copy link
Contributor Author

Okay, cool - I'll try to get the missing coverage for hooks today, so we can make sure this stuff doesn't break again 🙂

yisar added a commit that referenced this issue Oct 16, 2019
yisar added a commit that referenced this issue Oct 16, 2019
@yisar
Copy link
Collaborator

yisar commented Oct 16, 2019

It seems that I have fixed this bug, and then it should be #80

@mindplay-dk
Copy link
Contributor Author

I'm sorry, I think you misunderstood.

The test was correct - it failed because the effects weren't being triggered correctly.

image

Now the test is wrong: the same cleanup and effects should not execute more than once!

@yisar
Copy link
Collaborator

yisar commented Oct 16, 2019

Can you give me an example with sandbox? the test I am not sure weather it is correct.

@yisar
Copy link
Collaborator

yisar commented Oct 16, 2019

In fact, the bug here has nothing to do with useeffect, but all my debugging results are the same as react. I need an example to show the difference between react and fre, not the document or the test ,Thank you very much

@yisar
Copy link
Collaborator

yisar commented Oct 16, 2019

Let me explain again. I'm not sure whether the test case is correct or not, but I won't say it's wrong. I have to get an example that can be debugged, or I will maintain the status quo. If the example proves my mistake, it's not difficult to fix it, please forgive me.

@mindplay-dk
Copy link
Contributor Author

I have to get an example that can be debugged, or I will maintain the status quo.

That's what the tests are for 😉

I guess from the HTML files in the demo folder that you're debugging from an HTML page in a browser?

When you run the tests under node, you can connect (for example from VS Code or WebStorm, whatever you're using) to the debugger, set breakpoints, inspect variables, run individual tests with a click, and so on - it's definitely worth learning. 🙂

@mindplay-dk
Copy link
Contributor Author

With your last commit, I can confirm that my sandbox example is now working correctly!

It took me a while to figure out why the test is passing with your change - it looks wrong, but now I see the error in my test! PR on the way! 😁

@mindplay-dk
Copy link
Contributor Author

mindplay-dk commented Oct 17, 2019

For the record, the weird problem I showed above where the scheduler seemed to die - this also no longer seems to occur, so these were probably related. 👍

@yisar
Copy link
Collaborator

yisar commented Oct 17, 2019

I published v1.8.5👏

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