Resolve double mounting error with Turbo(links) #1421
Conversation
Judahmeek
force-pushed
the
judahmeek/turbolinks
branch
from
24432ae
to
467b032
Compare
| @@ -61,7 +61,8 @@ The best source of docs is the main [ReactOnRails.js](https://github.com/shakaco | |||
| /** | |||
| * Set options for ReactOnRails, typically before you call ReactOnRails.register | |||
| * Available Options: | |||
| * `traceTurbolinks: true|false Gives you debugging messages on Turbolinks events | |||
| * `traceTurbolinks: true|false` Gives you debugging messages on Turbolinks events | |||
| * `turbo: true|false` enables Turbo, which is not auto-detected like the Turbolinks library is | |||
There was a problem hiding this comment.
@Judahmeek do we need to update this here as it doesn't seem to include turbo?
There was a problem hiding this comment.
we could allow for either string, but it's not totally required.
There was a problem hiding this comment.
Yeah I was thinking turbo?: boolean
Or turbo: boolean | null or however it's actually defined.
I think it'd help increase visibility because my IDE only shows traceTurboLinks.
I'm speaking specifically about the d.ts file.
There was a problem hiding this comment.
if we could easily just have the shorter traceTurbo, that's better for the long-term.
There was a problem hiding this comment.
@Judahmeek, yes, we need to update the d.ts file.
There was a problem hiding this comment.
@justin808 my interpretation of the code seems slightly different.
For Turbo to work I need:
turbo: true
To see the logging I need:
traceTurboLinks: true
Both have to be on for it to work and log with Turbo.
--
Turbolinks seems to work without opting into it. traceTurboLinks: true just prints to the logs in that scenario.
|
CI is failing. |
| @@ -61,7 +61,8 @@ The best source of docs is the main [ReactOnRails.js](https://github.com/shakaco | |||
| /** | |||
| * Set options for ReactOnRails, typically before you call ReactOnRails.register | |||
| * Available Options: | |||
| * `traceTurbolinks: true|false Gives you debugging messages on Turbolinks events | |||
| * `traceTurbolinks: true|false` Gives you debugging messages on Turbolinks events | |||
| * `turbo: true|false` enables Turbo, which is not auto-detected like the Turbolinks library is | |||
There was a problem hiding this comment.
@Judahmeek, yes, we need to update the d.ts file.
| document.addEventListener('turbolinks:render', reactOnRailsPageLoaded); | ||
| 'turbolinks:before-visit and turbolinks:load.'); | ||
| document.addEventListener('turbolinks:before-visit', reactOnRailsPageUnloaded); | ||
| document.addEventListener('turbolinks:load', reactOnRailsPageLoaded); |
There was a problem hiding this comment.
@Judahmeek did you test on Turbolinks 5?
|
CI was failing due to webpack-contrib/mini-css-extract-plugin#899. I can put the fix commit in a separate PR if you'd prefer, but I'm expecting a new version from mini-css-extract-plugin's maintainers pretty quick since they already merged the fix. |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Judahmeek)
CHANGELOG.md, line 19 at r2 (raw file):
### Fixed - Double rendering of components with using Turbo or Turbolinks. [PR 1421](https://github.com/shakacode/react_on_rails/pull/1421) by [judahmeek](https://github.com/judahmeek).
and TS type for turbo option
node_package/src/clientStartup.ts, line 253 at r1 (raw file):
Previously, Judahmeek (Judah Meek) wrote…
I did.
👍
There was a problem hiding this comment.
@Judahmeek up to you. After you make the changelog change, you can merge, squashing commits with a nice commit message.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Judahmeek)
|
@justin808, @templeman15 says that this fix created a new problem with flashing content. We're exploring alternative solutions, such as #1425. Also, I don't have permissions to merge PRs to |
Fixes #1139
This change is