bug: Prevent roots from being re-created when using React 18 #1305

diogobeda · 2023-08-31T13:58:58Z

Summary

When dynamically adding components to a page by using, for example, Turbo streams and either manually calling ReactRailsUJS.mountComponents or passing ReactRailsUJS.handleMount to a Turbo event, there's a bug where the react roots are re-created on a node instead of being re-used. That resets the local state of components that were already mounted and may also cause visual glitches with the component flashing.

Other Information

I made a reproduction repo to showcase the bug happening and as a test case for the fix. Feel free to try it out for yourselves.

Here's a gif from before the fix:

And one from after the fix:

Please let me know if there's anything else I should add to this PR 🙏

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

Add/update test to cover these changes
Update documentation
Update CHANGELOG file

CHANGELOG.md

justin808 · 2023-09-08T20:29:43Z

react_ujs/index.js

      }
-    )
+    );


Is it possible to have a test that fails without this change? And passes with this change?

Can you extract some code from

I made a reproduction repo to showcase the bug happening and as a test case for the fix. Feel free to try it out for yourselves.

into https://github.com/reactjs/react-rails/blob/master/test/dummy

and add a test?

Sounds good! I'll do that in the next few days.

Hey @justin808, the dummy project's appraisals are missing some of the dependencies I've used in the reproduction case, such as Turbo and sqlite for creating the Timer records. Can I add those or would you prefer another approach?

@diogobeda feel free to add them.

justin808 · 2023-10-10T21:30:20Z

@diogobeda let me know when you added tests.

We need CI to pass.

justin808 · 2023-10-16T23:58:41Z

@diogobeda, I'd like to merge. Let me know when you added tests.

We need CI to pass.

diogobeda · 2023-10-26T12:46:49Z

Hey @justin808, sorry! I haven't been having much time to work on this, but I'll try to prioritize this for next week.

justin808 · 2023-11-07T09:21:26Z

@diogobeda thanks! Let's get this PR merged!

diogobeda · 2023-11-10T12:15:28Z

@justin808 added a test case and confirmed it failed on master with React 18, so we should be good to go if there's nothing else to fix on CI

justin808 · 2023-11-17T07:36:40Z

@ahangarha @Judahmeek @diogobeda are the CI errors legit?

diogobeda · 2023-11-21T15:35:41Z

I was trying these out locally and I can see the failures but I'm not sure what's wrong because I can't debug them for some reason. It's hard trying to log things in those tests and I'm not sure what to do next for them, so I could use some help.

justin808 · 2023-11-23T02:33:16Z

Seem integration tests fail with Sprockets 3 and 4, but work with shakapacker.

@diogobeda does that help?

See

https://github.com/reactjs/react-rails/blob/master/.github/workflows/ruby.yml#L61

https://github.com/reactjs/react-rails/blob/master/gemfiles/sprockets_3.gemfile

diogobeda · 2023-12-04T19:53:44Z

I get the following error with running the test suite for both sprockets_3 and sprockets_4:

bundle exec appraisal sprockets_3 rake test
>> BUNDLE_GEMFILE=/Users/diogobeda/workspace/podia/react-rails/gemfiles/sprockets_3.gemfile bundle exec rake test
warning package.json: No license field
warning No license field
Coverage report generated for Unit Tests to /Users/diogobeda/workspace/podia/react-rails/coverage. 659 / 1641 LOC (40.16%) covered.
Run options: --seed 45280

# Running:

dyld[54325]: missing symbol called
rake aborted!
Command failed with status (): [/Users/diogobeda/.asdf/installs/ruby/2.7.8...]
/Users/diogobeda/.asdf/installs/ruby/2.7.8/bin/bundle:23:in `load'
/Users/diogobeda/.asdf/installs/ruby/2.7.8/bin/bundle:23:in `<main>'
Tasks: TOP => test
(See full trace by running task with --trace)

My ruby version is:

ruby -v
ruby 2.7.8p225 (2023-03-30 revision 1f4d455848) [arm64-darwin22]

My node version is:

node -v
v18.18.2

I've researched for what the error could be and the few results I can find are M1/M2 related, so I've tried running with arch -x86_64 to no avail:

arch -x86_64  bundle exec appraisal sprockets_4 rake test
>> BUNDLE_GEMFILE=/Users/diogobeda/workspace/podia/react-rails/gemfiles/sprockets_4.gemfile bundle exec rake test
warning package.json: No license field
warning No license field
Coverage report generated for Unit Tests to /Users/diogobeda/workspace/podia/react-rails/coverage. 659 / 1625 LOC (40.55%) covered.
Run options: --seed 9016

# Running:

dyld[55666]: missing symbol called
rake aborted!
Command failed with status (): [/Users/diogobeda/.asdf/installs/ruby/2.7.8...]
/Users/diogobeda/.asdf/installs/ruby/2.7.8/bin/bundle:23:in `load'
/Users/diogobeda/.asdf/installs/ruby/2.7.8/bin/bundle:23:in `<main>'
Tasks: TOP => test
(See full trace by running task with --trace)

@justin808 would you be willing to pair on this some time this week?

justin808 · 2023-12-04T22:02:52Z

@diogobeda please join our slack for react-rails and ping @Judahmeek to pair on this.

I truly appreciate your help!

justin808 · 2023-12-29T23:58:35Z

@ahangarha @G-Rath should this be merged once CI passes? Might be the same issue at #1306 for CI.

G-Rath · 2024-01-01T23:04:11Z

The CI failures here look different to the ones in #1306 - it does look like I broke CI though in #1302 which I've opened #1328 to revert; that might unblock this once landed, but either way it should put us in a better state.

justin808 · 2024-01-06T07:38:08Z

@diogobeda can you rebase on master?

justin808 · 2024-01-10T05:05:01Z

I'd love to get this merged, but we have spec failures and rubocop issues.

ahangarha · 2024-01-10T13:52:25Z

Rubocop issues are easy to fix.

For the failures related to not finding logs methods, it seems we should consider #1326 again.

@G-Rath What do you think?

G-Rath

I'm not actually able to run the full test suite locally due to my setup (it requires a higher version of selenium-webdrivers than what's supported by Ruby 2.7), but besides the logs error (which I've suggested a fix for) I'm not see anything indicating this isn't just a general issue in your tests since they're now passing on the default branch.

It would be good if someone who can run the full test suite locally could confirm if this errors are reproducable.

I'm also curious to why you're only installing turbo-rails for the shakapacker gemfile?

G-Rath · 2024-01-10T17:41:26Z

test/react/rails/realtime_update_test.rb

+    def assert_counter_count(page, timer_name, count)
+      assert page.has_content?("#{timer_name} - #{count}"), <<~MSG
+        #{page.body}
+        #{page.driver.browser.logs.get(:browser).inspect}


This is what's causing the log error:

Suggested change

#{page.driver.browser.logs.get(:browser).inspect}

#{page.driver.browser.manage.logs.get(:browser).inspect}

And using headless=new option for the driver can fix this issue. Why not apply?

I did recommend applying it back when we were doing the CI fixing PRs, but we ended up going the other direction.

Overall that change is out of scope for this PR, but I also think it might also not be possible on the current gem versions which is why I've not done a PR myself - I'm happy to review a PR if you want to give it a try, but it'll eventually get done after dropping support for Ruby 2.7 etc if it's not done before then.

justin808 · 2024-01-15T08:03:22Z

@diogobeda can you please check test failures?

diogobeda · 2024-01-18T21:17:19Z

Hey @justin808 @G-Rath, I'm gonna close this PR because it's been too hard to deal with the project's test suite and I can't keep on top of it. Feel free to take my commits/changes and run with it, but I can no longer contribute.

justin808 · 2024-01-26T06:17:06Z

Reopen so I don't forget!

Prevent root from being recreated on react 18 (recreation of #1305)

justin808 reviewed Sep 1, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

diogobeda requested a review from justin808 September 6, 2023 13:11

diogobeda changed the title ~~Prevent roots from being re-created when using React 18~~ bug: Prevent roots from being re-created when using React 18 Sep 6, 2023

justin808 requested changes Sep 8, 2023

View reviewed changes

diogobeda force-pushed the master branch 2 times, most recently from 89cdc9f to e9d1b5e Compare November 10, 2023 12:14

G-Rath mentioned this pull request Jan 2, 2024

ci: revert lockfile updates and don't allow lockfile updates when installing #1328

Merged

3 tasks

diogobeda force-pushed the master branch from e9d1b5e to b05c8a7 Compare January 8, 2024 13:42

G-Rath suggested changes Jan 10, 2024

View reviewed changes

diogobeda closed this Jan 18, 2024

justin808 reopened this Jan 26, 2024

ahangarha self-assigned this Mar 6, 2024

Judahmeek closed this Apr 12, 2024

Judahmeek force-pushed the master branch from b05c8a7 to 6b65463 Compare April 12, 2024 22:58

Judahmeek mentioned this pull request Apr 12, 2024

Prevent root from being recreated on react 18 (recreation of #1305) #1335

Merged

2 tasks

justin808 added a commit that referenced this pull request May 16, 2024

Merge pull request #1335 from reactjs/diogo

116da19

Prevent root from being recreated on react 18 (recreation of #1305)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug: Prevent roots from being re-created when using React 18 #1305

bug: Prevent roots from being re-created when using React 18 #1305

diogobeda commented Aug 31, 2023 •

edited

Loading

justin808 Sep 8, 2023

diogobeda Sep 13, 2023

diogobeda Sep 29, 2023

justin808 Oct 6, 2023

justin808 commented Oct 10, 2023

justin808 commented Oct 16, 2023

diogobeda commented Oct 26, 2023

justin808 commented Nov 7, 2023

diogobeda commented Nov 10, 2023

justin808 commented Nov 17, 2023

diogobeda commented Nov 21, 2023

justin808 commented Nov 23, 2023

diogobeda commented Dec 4, 2023

justin808 commented Dec 4, 2023

justin808 commented Dec 29, 2023

G-Rath commented Jan 1, 2024

justin808 commented Jan 6, 2024

justin808 commented Jan 10, 2024

ahangarha commented Jan 10, 2024

G-Rath left a comment

G-Rath Jan 10, 2024

ahangarha Jan 15, 2024

G-Rath Jan 15, 2024

justin808 commented Jan 15, 2024

diogobeda commented Jan 18, 2024

justin808 commented Jan 26, 2024

	#{page.driver.browser.logs.get(:browser).inspect}
	#{page.driver.browser.manage.logs.get(:browser).inspect}

bug: Prevent roots from being re-created when using React 18 #1305

bug: Prevent roots from being re-created when using React 18 #1305

Conversation

diogobeda commented Aug 31, 2023 • edited Loading

Summary

Other Information

Pull Request checklist

justin808 Sep 8, 2023

Choose a reason for hiding this comment

diogobeda Sep 13, 2023

Choose a reason for hiding this comment

diogobeda Sep 29, 2023

Choose a reason for hiding this comment

justin808 Oct 6, 2023

Choose a reason for hiding this comment

justin808 commented Oct 10, 2023

justin808 commented Oct 16, 2023

diogobeda commented Oct 26, 2023

justin808 commented Nov 7, 2023

diogobeda commented Nov 10, 2023

justin808 commented Nov 17, 2023

diogobeda commented Nov 21, 2023

justin808 commented Nov 23, 2023

diogobeda commented Dec 4, 2023

justin808 commented Dec 4, 2023

justin808 commented Dec 29, 2023

G-Rath commented Jan 1, 2024

justin808 commented Jan 6, 2024

justin808 commented Jan 10, 2024

ahangarha commented Jan 10, 2024

G-Rath left a comment

Choose a reason for hiding this comment

G-Rath Jan 10, 2024

Choose a reason for hiding this comment

ahangarha Jan 15, 2024

Choose a reason for hiding this comment

G-Rath Jan 15, 2024

Choose a reason for hiding this comment

justin808 commented Jan 15, 2024

diogobeda commented Jan 18, 2024

justin808 commented Jan 26, 2024

diogobeda commented Aug 31, 2023 •

edited

Loading