Skip to content
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

Fix no-turbolinks ci #244

Merged
merged 18 commits into from
Feb 1, 2016
Merged

Fix no-turbolinks ci #244

merged 18 commits into from
Feb 1, 2016

Conversation

justin808
Copy link
Member

Root cause of failures on CI might be that if gem turbolinks is installed, then we don't run the handlers for no-turbolinks.

However, that should only be affected by requiring of turbolinks.

Maybe we should change:

  1. Don't worry if the Gem turbolinks is installed.
  2. Only check the JS side if turbolinks is running.

Review on Reviewable

@justin808
Copy link
Member Author

@robwise You might want to refactor this a tiny bit.

@robwise
Copy link
Contributor

robwise commented Jan 30, 2016

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


rakelib/dummy_apps.rake, line 11 [r1] (raw file):
there's a helper in task_helpers.rb that we should use for this to keep it DRY, not sure why I didn't use that in the first place


rakelib/run_rspec.rake, line 26 [r1] (raw file):
why are we bundle installing after we run the tests?


Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


rakelib/dummy_apps.rake, line 11 [r1] (raw file):
agree!


rakelib/run_rspec.rake, line 26 [r1] (raw file):
return to prior state or else gemfile.lock is dirty


Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Jan 30, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


rakelib/run_rspec.rake, line 26 [r1] (raw file):
ah, makes sense! I think this calls for a comment here because otherwise we might forget or someone else might not know etc.


Comments from the review on Reviewable.io

justin808 and others added 7 commits January 29, 2016 23:45
And don't bundle with the option DISABLE_TURBOLINKS on.
ROR in SERVER PRERENDERING
Encountered error: "TypeError: Cannot read property 'name' of undefined"
when prerendering N/A (server_render_js called) with props:
js_code was:
(function() {
  var htmlResult = '';
  var consoleReplayScript = '';
  var hasErrors = false;

  try {
    htmlResult =
      (function() {
        return
ReactOnRails.getComponent('HelloString').component.world();
      })();
  } catch(e) {
    htmlResult = ReactOnRails.handleError({e: e, name: null,
      jsCode:
'ReactOnRails.getComponent(\'HelloString\').component.world()',
serverSide: true});
    hasErrors = true;
  }

  consoleReplayScript = ReactOnRails.buildConsoleReplay();

  return JSON.stringify({
      html: htmlResult,
      consoleReplayScript: consoleReplayScript,
      hasErrors: hasErrors
  });

})()

Object.<anonymous> ((execjs):27066:58)
Object.<anonymous> ((execjs):27075:31)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):26981:15)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):5355:24)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):59:19)
__webpack_require__ ((execjs):31:30)
(execjs):51:18
Probably not related to the react version

Encountered error: "TypeError: Cannot read property 'name' of undefined"
when prerendering HelloWorldWithLogAndThrow with props:
{"helloWorldData":{"name":"Mr. Server Side Rendering"}}
js_code was:
(function() {
  var props = {"helloWorldData":{"name":"Mr. Server Side Rendering"}};
  return ReactOnRails.serverRenderReactComponent({
    name: 'HelloWorldWithLogAndThrow',
    domNodeId: 'HelloWorldWithLogAndThrow-react-component-0',
    props: props,
    trace: true,
    location: '/server_side_log_throw_raise'
  });
})()

Object.<anonymous> ((execjs):27066:58)
Object.<anonymous> ((execjs):27075:31)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):26981:15)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):5355:24)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):59:19)
__webpack_require__ ((execjs):31:30)
(execjs):51:18
This change prints a useful message and outputs the file with the issue.
@justin808
Copy link
Member Author

Debugging details:

#246 (comment)

@justin808
Copy link
Member Author

@justin808
Copy link
Member Author

See reduxjs/redux#1335

@robwise
Copy link
Contributor

robwise commented Jan 31, 2016

Reviewed 8 of 8 files at r2, 1 of 4 files at r4, 4 of 4 files at r5.
Review status: 17 of 18 files reviewed at latest revision, 2 unresolved discussions.


rakelib/dummy_apps.rake, line 10 [r1] (raw file):
can we put this back now or this was breaking CI also?


Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Jan 31, 2016

Reviewed 1 of 4 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Feb 1, 2016

Reviewed 4 of 4 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

👍😎


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


rakelib/dummy_apps.rake, line 10 [r1] (raw file):
we just don't need separate bundle install


Comments from the review on Reviewable.io

@alex35mil
Copy link
Member

:lgtm_strong:


Review status: 20 of 21 files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

justin808 added a commit that referenced this pull request Feb 1, 2016
@justin808 justin808 merged commit 33a9262 into master Feb 1, 2016
@justin808 justin808 deleted the fix-ci-no-turbolinks branch February 1, 2016 09:22
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