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

fix: Prevent infinite loop with React 16 refs #357

Merged
merged 2 commits into from
Jul 19, 2019
Merged

fix: Prevent infinite loop with React 16 refs #357

merged 2 commits into from
Jul 19, 2019

Conversation

skvale
Copy link
Contributor

@skvale skvale commented Mar 10, 2019

Relates to #344

This provides a reproduction of the bug with a test.

Remove this block

 if (key === 'current') {
        // eslint-disable-next-line no-param-reassign
        result[key] = '[Circular]';
 }

and it will fail with the Maximum call stack size exceeded going into the circular

element->ref->current->element->ref->current

loop.

@vvo
Copy link
Contributor

vvo commented Mar 11, 2019

Hey there, thanks, I was trying to fix your tests, and thought that it was because of incompat issues with latest nodejs, so I created: #358

but then there's another test error we have seen in the past already (#340) cc @Spy-Seth à l'aide :D (help us)

@armandabric
Copy link
Collaborator

armandabric commented Mar 11, 2019

😭😭😭😭

I will not be able to have a look this week 😢

@skvale skvale changed the title fix: Add a test for issue 344 fix: Prevent infinite loop with React 16 refs Mar 21, 2019
@armandabric armandabric requested review from vvo and armandabric May 12, 2019 12:08
@armandabric armandabric removed their assignment May 12, 2019
@armandabric
Copy link
Collaborator

armandabric commented May 12, 2019

Fixing #358 and rebasing this PR make it green.

Not sure this is a good thing: it means our code try to write on read only object attributes sometimes and node v12 now trigger error on CI (not on my laptop...). We should investigate this

@hipstersmoothie
Copy link

hipstersmoothie commented Jul 18, 2019

any progress on this?

Edit: I've tested in my storybook and the change fixes the issue

@vvo
Copy link
Contributor

vvo commented Jul 19, 2019

I see this code is now working on Node v12 so we're good to go

@vvo vvo merged commit 5fe7604 into algolia:master Jul 19, 2019
@vvo
Copy link
Contributor

vvo commented Jul 19, 2019

released as 14.0.3

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

Successfully merging this pull request may close these issues.

5 participants