Fix server-side rendering in development mode #59
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! |
|
I'm not a fan of the sleep(1) calls in the test file I added, but I couldn't get Rails / PhantomJS to reliably pick up that there were changes without adding some delays. If anyone has suggestions on better ways to go about testing this, let me know. Also, in case the comment wasn't clear: there's a bug in server-side rendering in development. It currently caches the concatenated components the first time their used, and doesn't get updates correctly, which breaks the server rendering. React is smart enough to take over when the HTML hits the browser and replace the outdated HTML with new HTML, but it means that you're not actually using server-side rendering in development. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
xionon
changed the title
|
Regarding the test failure on Travis, I'm getting inconsistent failures depending on the order the tests are run, and it seems to be unrelated to my patch, and more of a PhantomJS issue. I'm not very familiar with PhantomJS, and would appreciate it if anybody had any ideas. |
|
Inconsistent tests results and random timeouts are probably related. When I run locally I'm seeing JS errors ( |
|
@zpao Yep, I've seen that error, too. Should I make a separate issue for that problem? I think it's unrelated. On react-rails/master (i.e., without this patch applied), I can run the tests 10 times in a row, 5 times it will fail, 5 times it will pass. Test::Unit randomizes the order the tests are run in, so it could be a weird dependency issue between multiple tests. |
|
Added |
|
Ahh, my bad, upstream changes break mine. I'll re-open once fixed, if it's even necessary anymore. |
|
OK, I think I finally fixed this! The issue was pretty gnarly, hopefully I can explain it clearly. The test inconsistencies were related to the way that Rails' Asset Pipeline detects file changes: Rails looks at an asset file's mtime, which tells it if the file has been updated, and if it has the various setup blocks are run and the asset is re-compiled. However, mtime has a resolution of only 1 second, which means that if several operations on a file happen in less than a second, the mtime won't appear to have updated, and the asset pipeline can't detect it and refresh the file list. Some of the tests modify the react.js or components files, and then issue a GET to ensure that the contents of that file or the server rendered markup updated correctly. For example, given these two tests: If Test A runs before Test B, no problem. However, if Test B runs first, the mtime of the react.js file won't be updated when Test B cleans itself up - which means that Test A will get the fake string instead of the actual react.js code, which will cause it to error out. I was seeing these errors both on master and on my branch. If you want me to pull this out into a separate pull request, I'm happy to do that. |
|
Also, if you want to run the test several times in sequence to get different seeds and see if your tests fail depending on the order they run in, you can use: |
|
Could you post the seed that fails without your fix? |
|
@JakubMal I only thought to save a seed that would fail reliably on my branch. I don't have a seed that reliably fails on master. Using Ruby 1.9.3, on xionon/react-rails, commit 1c1ab82, you can reliably get test failures with Switching to xionon/react-rails commit 6764630 fixes this |
|
I removed the sleep(0.1) call in test/view_helper_test.rb, because I can't reliably reproduce the test bug that I was hoping to fix. followed by -- same seed, same gemfile. First run fails, second run succeeds. Gah. |
There's a timing bug in ViewHelperTest that randomly results in ```ReferenceError: Turbolinks Not Found```. This causes unrelated builds to fail, which gums up the whole open source works. Per https://github.com/teampoltergeist/poltergeist#timing-problems, the suggested way of fixing this is to just add calls to sleep. If you wish to test this in the future, use the shell code: ```for i in `seq 1 10`; do rake appraisal; done``` This should give you enough test runs to hit the timing bug at least once.
The renderer was using a memoized class variable to cache the concatinated react.js and components.js for server-side rendering. This meant that when you updated a component, the server-rendered version would not be refreshed. Since #setup! is called every time a watched file is updated, it seems like the natural place to reset the memozied string.
Sometimes Turbolinks hasn't fully loaded before Capybara attempts to call 'Turbolinks.visit'. Adding a brief sleep gives Turbolinks a chance to become availabe to PhantomJS
Background: Running tests multiple times in a row could result in errors, and could result in complete passing. In other words, the order the tests ran in determined whether or not the tests would pass. Ruby Test::Unit uses a psuedorandom seed to randomize the order that tests are run in; you can specify a certain seed with TESTOPS="--seed=xyz", which fixes the seed and thus the order the tests will be run in. The main source of the bug was that many of the tests rely on replacing asset files and asserting that the results are as expected; however, the asset pipeline uses mtime to determine when files have been changed. *mtime's resolution is only 1 second, meaning that when a series of actions took less than a second to run, the mtime wouldn't actually change, and Rails asset pipeline wouldn't update the file list.* This commit adds a hook to force reset the internal concatinated react.js and components.js string. Another test requires a few calls to sleep(1), because it is specifically testing that the asset pipeline picks up changes to the file on disk correctly and renders them server side.
Since I can't reliably reproduce the ```Turbolinks Not Found``` error, this code is basically just hopeful thinking.
This reverts commit 55d2a1d.
Really just trying to work around an incredibly annoying bug that only seems to crop up once in a while.
|
Sure, let's do it. |
Fix server-side rendering in development mode
|
I still get the problem when add a component, but I find a way to resolve this. |
The renderer was using a memoized class variable to cache the
concatinated react.js and components.js for server-side rendering. This
meant that when you updated a component, the server-rendered version would
not be refreshed.
Since #setup! is called every time a watched file is updated, it seems
like the natural place to reset the memozied string.