expose reactOnRailsPageLoaded to ReactOnRails #315
Conversation
SqueezedLight
commented
Mar 7, 2016
SqueezedLight
commented
|
Looking great! Reviewed 4 of 4 files at r1. docs/additional_reading/turbolinks.md, line 39 [r1] (raw file): docs/additional_reading/turbolinks.md, line 46 [r1] (raw file): node_package/src/ReactOnRails.js, line 64 [r1] (raw file): import * as ClientStartup from './clientStartup';And then we can use ClientStartup.clientStartup()
ClientStartup.reactOnRailsPageLoaded()Any suggestions? spec/dummy/Gemfile.lock, line 75 [r1] (raw file): Comments from the review on Reviewable.io |
|
Review status: all files reviewed at latest revision, 4 unresolved discussions. docs/additional_reading/turbolinks.md, line 46 [r1] (raw file): Comments from the review on Reviewable.io |
|
Review status: all files reviewed at latest revision, 4 unresolved discussions. docs/additional_reading/turbolinks.md, line 39 [r1] (raw file): docs/additional_reading/turbolinks.md, line 46 [r1] (raw file): spec/dummy/Gemfile.lock, line 75 [r1] (raw file): Comments from the review on Reviewable.io |
|
Reviewed 2 of 2 files at r2. node_package/src/ReactOnRails.js, line 64 [r2] (raw file): Comments from the review on Reviewable.io |
|
Let's address the "can we change the clientStartup method" to solve so of the TurboLinks issues. Thanks! Review status: all files reviewed at latest revision, 4 unresolved discussions. docs/additional_reading/turbolinks.md, line 39 [r1] (raw file): However I think we should be supporting the turbolinks part without the need to have the extra code. The problem is that the library won't know (at least I think not) if the script was loaded async or defer. docs/additional_reading/turbolinks.md, line 46 [r1] (raw file): Comments from the review on Reviewable.io |
|
@SqueezedLight I'm going to try looking into this issue over the weekend. If you have any additional info, please let me know. |
|
@SqueezedLight Please rebase on top of master to a single commit and let me know if you're ready. I'm about to release v4. |
|
+1 |
|
@justin808 sorry for the delay again. All done! |
|
Given turbolinks/turbolinks#28 I'm still not totally sure on the docs for this feature. I'm concerned that somebody will be running the loaded handler code twice in some cases. Thoughts? |
|
In the docs i wrote: "When loading your scripts asynchronously you may experience, that your Components are not registered correctly. Call If i understand you correctly you insist on making it much more clear that this is optional and only in case components are not registered correctly, right? To be honest i'm feeling like i do not really get the severeness of your concerns ;) What i do know is, that without |
It's not about "registration" but script loading settings and Turbolinks, which I'm researching heavily this week. Possibly the version of Turbolinks matters (2.x or 5.x)
That's good. I'm going to take the API docs, ruby and JS, and put them into separate files, very soon.
Basically, the gem has gotten pretty complicated. For every new option we add, a newbie must consider "do I need to use that option?". One of the goals of ReactOnRails is that the component loading should just work. Basically, I can handle the remaining to-do's (but help is always appreciated):
|
|
@jbhatab @alexfedoseev @thewoolleyman @robwise Agree? |
|
@SqueezedLight Any word? I really want to accept this, but I'm mostly concerned that we'll try to double call the render sometimes. This code runs when the JS from the bundle is loaded. It installs the handler. I'm thinking that if we install turbolinks via npm in the client bundles, then we won't need this. export default function clientStartup(context) {
const document = context.document;
// Check if server rendering
if (!document) {
return;
}
// Tried with a file local variable, but the install handler gets called twice.
if (context.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__) {
return;
}
context.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__ = // eslint-disable-line no-param-reassign
true;
debugTurbolinks('Adding DOMContentLoaded event to install event listeners.');
document.addEventListener('DOMContentLoaded', () => {
// Install listeners when running on the client (browser).
// We must do this check for turbolinks AFTER the document is loaded because we load the
// Webpack bundles first.
if (!turbolinksInstalled()) {
debugTurbolinks(
'NOT USING TURBOLINKS: DOMContentLoaded event, calling reactOnRailsPageLoaded'
);
reactOnRailsPageLoaded();
} else {
if (turbolinksVersion5()) {
debugTurbolinks(
'USING TURBOLINKS 5: document added event listeners turbolinks:before-cache and ' +
'turbolinks:load.'
);
document.addEventListener('turbolinks:before-cache', reactOnRailsPageUnloaded);
document.addEventListener('turbolinks:load', reactOnRailsPageLoaded);
} else {
debugTurbolinks(
'USING TURBOLINKS 2: document added event listeners page:before-unload and ' +
'page:change.');
document.addEventListener('page:before-unload', reactOnRailsPageUnloaded);
document.addEventListener('page:change', reactOnRailsPageLoaded);
}
}
});
} |
|
@SqueezedLight I'm going to put a new release out. We need to rebase this on top of master make sure it passes CI. Any thoughts on my prior questions? |
|
@justin808 oh man i totally lost track of this issue. I'm under very heavy work load at the moment. To be honest i'm more or less unable to make a qualified statement to your questions. A lot has changed in this gem and the version i run is not the current one (3.0.6). I tried to update my code several times in the last 4 weeks - but it completely breaks a lot of (well established) things - asset loading for example. The gaps between releases are huge. Too huge for me to follow on the side. That is why i'm not in the position to make a helpful statement. Sorry for not being more of a help. |
|
@SqueezedLight I'm going to look at this more in the coming week. |
|
@SqueezedLight I should have time in the next few days to check this one out. If you can rebase on top of master, that would be appreciated! |
|
Closed as it's merged in #417 |
