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

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

Jump to bottom

Closed

diogobeda wants to merge 0 commits into reactjs:master from diogobeda:master

Closed

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

diogobeda wants to merge 0 commits into reactjs:master from diogobeda:master

Conversation

diogobeda

Copy link

@diogobeda diogobeda commented Aug 31, 2023 •

edited

Loading

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:

CleanShot 2023年08月31日 at 10 37 40

And one from after the fix:

CleanShot 2023年08月31日 at 10 40 58

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

justin808

justin808 reviewed

Sep 1, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

@diogobeda diogobeda requested a review from justin808

September 6, 2023 13:11

@diogobeda 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

justin808 requested changes

Sep 8, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

react_ujs/index.js Outdated

}

)

);

Copy link

Collaborator

@justin808 justin808 Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Author

@diogobeda diogobeda Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Author

@diogobeda diogobeda Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Collaborator

@justin808 justin808 Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diogobeda feel free to add them.

@justin808

Copy link

Collaborator

justin808 commented Oct 10, 2023

@diogobeda let me know when you added tests.

We need CI to pass.

@justin808

Copy link

Collaborator

justin808 commented Oct 16, 2023

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

We need CI to pass.

@diogobeda

Copy link

Author

diogobeda commented Oct 26, 2023

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

Copy link

Collaborator

justin808 commented Nov 7, 2023

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

@diogobeda diogobeda force-pushed the master branch 2 times, most recently from 89cdc9f to e9d1b5e Compare

November 10, 2023 12:14

@diogobeda

Copy link

Author

diogobeda commented Nov 10, 2023

@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

Copy link

Collaborator

justin808 commented Nov 17, 2023

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

@diogobeda

Copy link

Author

diogobeda commented Nov 21, 2023

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

Copy link

Collaborator

justin808 commented Nov 23, 2023

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

Copy link

Author

diogobeda commented Dec 4, 2023

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

Copy link

Collaborator

justin808 commented Dec 4, 2023

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

I truly appreciate your help!

@justin808

Copy link

Collaborator

justin808 commented Dec 29, 2023

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

@G-Rath

Copy link

Contributor

G-Rath commented Jan 1, 2024

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.

@G-Rath 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

@justin808

Copy link

Collaborator

justin808 commented Jan 6, 2024

@diogobeda can you rebase on master?

@diogobeda diogobeda force-pushed the master branch from e9d1b5e to b05c8a7 Compare

January 8, 2024 13:42

@justin808

Copy link

Collaborator

justin808 commented Jan 10, 2024

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

@ahangarha

Copy link

Collaborator

ahangarha commented Jan 10, 2024

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

G-Rath suggested changes

Jan 10, 2024

View reviewed changes

Copy link

Contributor

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

test/react/rails/realtime_update_test.rb Outdated

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}

Copy link

Contributor

@G-Rath G-Rath Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}

Copy link

Collaborator

@ahangarha ahangarha Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor

@G-Rath G-Rath Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Collaborator

justin808 commented Jan 15, 2024

@diogobeda can you please check test failures?

@diogobeda

Copy link

Author

diogobeda commented Jan 18, 2024

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.

@diogobeda diogobeda closed this

Jan 18, 2024

@justin808 justin808 reopened this

Jan 26, 2024

@justin808

Copy link

Collaborator

justin808 commented Jan 26, 2024

Reopen so I don't forget!

@ahangarha ahangarha self-assigned this

Mar 6, 2024

@Judahmeek Judahmeek closed this

Apr 12, 2024

@Judahmeek Judahmeek force-pushed the master branch from b05c8a7 to 6b65463 Compare

April 12, 2024 22:58

@Judahmeek 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

@justin808


 Merge pull request #1335 from reactjs/diogo

116da19

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

Labels

None yet

5 participants

@diogobeda @justin808 @G-Rath @ahangarha @Judahmeek

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

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

Uh oh!

Conversation

@diogobeda diogobeda commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Other Information

Pull Request checklist

Uh oh!

Uh oh!

Uh oh!

@justin808 justin808 Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

@diogobeda diogobeda Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

@diogobeda diogobeda Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

@justin808 justin808 Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

justin808 commented Oct 10, 2023

Uh oh!

justin808 commented Oct 16, 2023

Uh oh!

diogobeda commented Oct 26, 2023

Uh oh!

justin808 commented Nov 7, 2023

Uh oh!

diogobeda commented Nov 10, 2023

Uh oh!

justin808 commented Nov 17, 2023

Uh oh!

diogobeda commented Nov 21, 2023

Uh oh!

justin808 commented Nov 23, 2023

Uh oh!

diogobeda commented Dec 4, 2023

Uh oh!

justin808 commented Dec 4, 2023

Uh oh!

justin808 commented Dec 29, 2023

Uh oh!

G-Rath commented Jan 1, 2024

Uh oh!

justin808 commented Jan 6, 2024

Uh oh!

justin808 commented Jan 10, 2024

Uh oh!

ahangarha commented Jan 10, 2024

Uh oh!

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

Uh oh!

@G-Rath G-Rath Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

@ahangarha ahangarha Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

@G-Rath G-Rath Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

justin808 commented Jan 15, 2024

Uh oh!

diogobeda commented Jan 18, 2024

Uh oh!

justin808 commented Jan 26, 2024

Uh oh!

Uh oh!

@diogobeda diogobeda commented Aug 31, 2023 •

edited

Loading