Skip to content

Fix reactOnRailsPageUnloaded when there is no component on the page#1498

Merged
justin808 merged 2 commits intoshakacode:masterfrom
NhanHo:fix_page_unloaded_turbo
Dec 30, 2022
Merged

Fix reactOnRailsPageUnloaded when there is no component on the page#1498
justin808 merged 2 commits intoshakacode:masterfrom
NhanHo:fix_page_unloaded_turbo

Conversation

@NhanHo
Copy link
Copy Markdown
Contributor

@NhanHo NhanHo commented Dec 21, 2022

In reactOnRailsPageLoaded, if we finds no react component, the function return and context.roots is not initialized. This cause findContext().roots in reactOnRailsPageUnloaded to be undefined, and for .. of throws an exception.

This should mostly affect app using both hotwire and react_on_rails, as the recommended way to load js is to do the same file everywhere, otherwise the issue can be avoided by just not loading react_on_rails


This change is Reviewable

@davidalejandroaguilar
Copy link
Copy Markdown

Nice, I actually hit this while I was trying react_on_rails a couple days ago. It made me think it was unstable so decided not to use it.

@justin808
Copy link
Copy Markdown
Member

@NhanHo can you please add a CHANGELOG entry?

I'll merge as soon as this passes CI.

// If no react on rails components
if (!roots) return;

for (const root of roots) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks OK for me. @alexeyr-ci1 @alexeyr please review as you added the prior line for React 18.

This seems harmless, so I'll merge. Still, I'd like a post-merge review.

@NhanHo NhanHo force-pushed the fix_page_unloaded_turbo branch from ef684f9 to 70dac07 Compare December 24, 2022 02:06
@NhanHo
Copy link
Copy Markdown
Contributor Author

NhanHo commented Dec 24, 2022

@justin808 I fixed the linter issue and added changelog. If you decide to merge the other PR, you can close this one.

Thanks a lot for your work on Shakapacker and related projects.

@justin808
Copy link
Copy Markdown
Member

@NhanHo NhanHo force-pushed the fix_page_unloaded_turbo branch from f24aa88 to c6b4092 Compare December 28, 2022 13:16
@NhanHo
Copy link
Copy Markdown
Contributor Author

NhanHo commented Dec 28, 2022

@justin808 Updated

@justin808 justin808 merged commit 95438ad into shakacode:master Dec 30, 2022
@justin808
Copy link
Copy Markdown
Member

Thanks @NhanHo!

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 this pull request may close these issues.

3 participants