From 36f51b424adc2ba3da1c4724eeecef6f201865fd Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Tue, 11 Apr 2017 13:44:07 -0400 Subject: [PATCH 1/2] fix(turbolinks) use good turbolinks events --- lib/assets/javascripts/react_ujs.js | 135 +++++++++++++++++----------- react_ujs/dist/react_ujs.js | 135 +++++++++++++++++----------- react_ujs/src/events/turbolinks.js | 5 +- test/test_helper.rb | 2 +- 4 files changed, 166 insertions(+), 111 deletions(-) diff --git a/lib/assets/javascripts/react_ujs.js b/lib/assets/javascripts/react_ujs.js index 1ddb2089..bef64240 100644 --- a/lib/assets/javascripts/react_ujs.js +++ b/lib/assets/javascripts/react_ujs.js @@ -78,6 +78,34 @@ return /******/ (function(modules) { // webpackBootstrap /************************************************************************/ /******/ ([ /* 0 */ +/***/ (function(module, exports) { + +// Assume className is simple and can be found at top-level (window). +// Fallback to eval to handle cases like 'My.React.ComponentName'. +// Also, try to gracefully import Babel 6 style default exports +var topLevel = typeof window === "undefined" ? this : window; + +module.exports = function(className) { + var constructor; + // Try to access the class globally first + constructor = topLevel[className]; + + // If that didn't work, try eval + if (!constructor) { + constructor = eval(className); + } + + // Lastly, if there is a default attribute try that + if (constructor && constructor['default']) { + constructor = constructor['default']; + } + + return constructor; +} + + +/***/ }), +/* 1 */ /***/ (function(module, exports, __webpack_require__) { var nativeEvents = __webpack_require__(7) @@ -118,58 +146,26 @@ module.exports = function(ujs) { } -/***/ }), -/* 1 */ -/***/ (function(module, exports) { - -// Assume className is simple and can be found at top-level (window). -// Fallback to eval to handle cases like 'My.React.ComponentName'. -// Also, try to gracefully import Babel 6 style default exports -var topLevel = typeof window === "undefined" ? this : window; - -module.exports = function(className) { - var constructor; - // Try to access the class globally first - constructor = topLevel[className]; - - // If that didn't work, try eval - if (!constructor) { - constructor = eval(className); - } - - // Lastly, if there is a default attribute try that - if (constructor && constructor['default']) { - constructor = constructor['default']; - } - - return constructor; -} - - /***/ }), /* 2 */ -/***/ (function(module, exports) { +/***/ (function(module, exports, __webpack_require__) { + +// Make a function which: +// - First tries to require the name +// - Then falls back to global lookup +var fromGlobal = __webpack_require__(0) +var fromRequireContext = __webpack_require__(12) -// Load React components by requiring them from "components/", for example: -// -// - "pages/index" -> `require("components/pages/index")` -// - "pages/show.Header" -> `require("components/pages/show").Header` -// - "pages/show.Body.Content" -> `require("components/pages/show").Body.Content` -// module.exports = function(reqctx) { + var fromCtx = fromRequireContext(reqctx) return function(className) { - var parts = className.split(".") - var filename = parts.shift() - var keys = parts - // Load the module: - var component = reqctx("./" + filename) - // Then access each key: - keys.forEach(function(k) { - component = component[k] - }) - // support `export default` - if (component.__esModule) { - component = component["default"] + var component; + try { + // `require` will raise an error if this className isn't found: + component = fromCtx(className) + } catch (err) { + // fallback to global: + component = fromGlobal(className) } return component } @@ -202,9 +198,9 @@ var React = __webpack_require__(3) var ReactDOM = __webpack_require__(4) var ReactDOMServer = __webpack_require__(5) -var detectEvents = __webpack_require__(0) -var constructorFromGlobal = __webpack_require__(1) -var constructorFromRequireContext = __webpack_require__(2) +var detectEvents = __webpack_require__(1) +var constructorFromGlobal = __webpack_require__(0) +var constructorFromRequireContextWithGlobalFallback = __webpack_require__(2) var ReactRailsUJS = { // This attribute holds the name of component which should be mounted @@ -255,8 +251,8 @@ var ReactRailsUJS = { // the default is ReactRailsUJS.ComponentGlobal getConstructor: constructorFromGlobal, - loadContext: function(req) { - this.getConstructor = constructorFromRequireContext(req) + useContext: function(req) { + this.getConstructor = constructorFromRequireContextWithGlobalFallback(req) }, // Render `componentName` with `props` to a string, @@ -360,8 +356,9 @@ module.exports = { module.exports = { // Turbolinks 5+ got rid of named events (?!) setup: function(ujs) { - ujs.handleEvent('turbolinks:load', function() { ujs.mountComponents() }); - ujs.handleEvent('turbolinks:before-render', function() { ujs.unmountComponents() }); + ujs.handleEvent('DOMContentLoaded', function() { ujs.mountComponents() }) + ujs.handleEvent('turbolinks:render', function() { ujs.mountComponents() }) + ujs.handleEvent('turbolinks:before-render', function() { ujs.unmountComponents() }) }, } @@ -397,6 +394,36 @@ module.exports = { } +/***/ }), +/* 12 */ +/***/ (function(module, exports) { + +// Load React components by requiring them from "components/", for example: +// +// - "pages/index" -> `require("components/pages/index")` +// - "pages/show.Header" -> `require("components/pages/show").Header` +// - "pages/show.Body.Content" -> `require("components/pages/show").Body.Content` +// +module.exports = function(reqctx) { + return function(className) { + var parts = className.split(".") + var filename = parts.shift() + var keys = parts + // Load the module: + var component = reqctx("./" + filename) + // Then access each key: + keys.forEach(function(k) { + component = component[k] + }) + // support `export default` + if (component.__esModule) { + component = component["default"] + } + return component + } +} + + /***/ }) /******/ ]); }); \ No newline at end of file diff --git a/react_ujs/dist/react_ujs.js b/react_ujs/dist/react_ujs.js index 1ddb2089..bef64240 100644 --- a/react_ujs/dist/react_ujs.js +++ b/react_ujs/dist/react_ujs.js @@ -78,6 +78,34 @@ return /******/ (function(modules) { // webpackBootstrap /************************************************************************/ /******/ ([ /* 0 */ +/***/ (function(module, exports) { + +// Assume className is simple and can be found at top-level (window). +// Fallback to eval to handle cases like 'My.React.ComponentName'. +// Also, try to gracefully import Babel 6 style default exports +var topLevel = typeof window === "undefined" ? this : window; + +module.exports = function(className) { + var constructor; + // Try to access the class globally first + constructor = topLevel[className]; + + // If that didn't work, try eval + if (!constructor) { + constructor = eval(className); + } + + // Lastly, if there is a default attribute try that + if (constructor && constructor['default']) { + constructor = constructor['default']; + } + + return constructor; +} + + +/***/ }), +/* 1 */ /***/ (function(module, exports, __webpack_require__) { var nativeEvents = __webpack_require__(7) @@ -118,58 +146,26 @@ module.exports = function(ujs) { } -/***/ }), -/* 1 */ -/***/ (function(module, exports) { - -// Assume className is simple and can be found at top-level (window). -// Fallback to eval to handle cases like 'My.React.ComponentName'. -// Also, try to gracefully import Babel 6 style default exports -var topLevel = typeof window === "undefined" ? this : window; - -module.exports = function(className) { - var constructor; - // Try to access the class globally first - constructor = topLevel[className]; - - // If that didn't work, try eval - if (!constructor) { - constructor = eval(className); - } - - // Lastly, if there is a default attribute try that - if (constructor && constructor['default']) { - constructor = constructor['default']; - } - - return constructor; -} - - /***/ }), /* 2 */ -/***/ (function(module, exports) { +/***/ (function(module, exports, __webpack_require__) { + +// Make a function which: +// - First tries to require the name +// - Then falls back to global lookup +var fromGlobal = __webpack_require__(0) +var fromRequireContext = __webpack_require__(12) -// Load React components by requiring them from "components/", for example: -// -// - "pages/index" -> `require("components/pages/index")` -// - "pages/show.Header" -> `require("components/pages/show").Header` -// - "pages/show.Body.Content" -> `require("components/pages/show").Body.Content` -// module.exports = function(reqctx) { + var fromCtx = fromRequireContext(reqctx) return function(className) { - var parts = className.split(".") - var filename = parts.shift() - var keys = parts - // Load the module: - var component = reqctx("./" + filename) - // Then access each key: - keys.forEach(function(k) { - component = component[k] - }) - // support `export default` - if (component.__esModule) { - component = component["default"] + var component; + try { + // `require` will raise an error if this className isn't found: + component = fromCtx(className) + } catch (err) { + // fallback to global: + component = fromGlobal(className) } return component } @@ -202,9 +198,9 @@ var React = __webpack_require__(3) var ReactDOM = __webpack_require__(4) var ReactDOMServer = __webpack_require__(5) -var detectEvents = __webpack_require__(0) -var constructorFromGlobal = __webpack_require__(1) -var constructorFromRequireContext = __webpack_require__(2) +var detectEvents = __webpack_require__(1) +var constructorFromGlobal = __webpack_require__(0) +var constructorFromRequireContextWithGlobalFallback = __webpack_require__(2) var ReactRailsUJS = { // This attribute holds the name of component which should be mounted @@ -255,8 +251,8 @@ var ReactRailsUJS = { // the default is ReactRailsUJS.ComponentGlobal getConstructor: constructorFromGlobal, - loadContext: function(req) { - this.getConstructor = constructorFromRequireContext(req) + useContext: function(req) { + this.getConstructor = constructorFromRequireContextWithGlobalFallback(req) }, // Render `componentName` with `props` to a string, @@ -360,8 +356,9 @@ module.exports = { module.exports = { // Turbolinks 5+ got rid of named events (?!) setup: function(ujs) { - ujs.handleEvent('turbolinks:load', function() { ujs.mountComponents() }); - ujs.handleEvent('turbolinks:before-render', function() { ujs.unmountComponents() }); + ujs.handleEvent('DOMContentLoaded', function() { ujs.mountComponents() }) + ujs.handleEvent('turbolinks:render', function() { ujs.mountComponents() }) + ujs.handleEvent('turbolinks:before-render', function() { ujs.unmountComponents() }) }, } @@ -397,6 +394,36 @@ module.exports = { } +/***/ }), +/* 12 */ +/***/ (function(module, exports) { + +// Load React components by requiring them from "components/", for example: +// +// - "pages/index" -> `require("components/pages/index")` +// - "pages/show.Header" -> `require("components/pages/show").Header` +// - "pages/show.Body.Content" -> `require("components/pages/show").Body.Content` +// +module.exports = function(reqctx) { + return function(className) { + var parts = className.split(".") + var filename = parts.shift() + var keys = parts + // Load the module: + var component = reqctx("./" + filename) + // Then access each key: + keys.forEach(function(k) { + component = component[k] + }) + // support `export default` + if (component.__esModule) { + component = component["default"] + } + return component + } +} + + /***/ }) /******/ ]); }); \ No newline at end of file diff --git a/react_ujs/src/events/turbolinks.js b/react_ujs/src/events/turbolinks.js index d1b36ec2..59197b4a 100644 --- a/react_ujs/src/events/turbolinks.js +++ b/react_ujs/src/events/turbolinks.js @@ -1,7 +1,8 @@ module.exports = { // Turbolinks 5+ got rid of named events (?!) setup: function(ujs) { - ujs.handleEvent('turbolinks:load', function() { ujs.mountComponents() }); - ujs.handleEvent('turbolinks:before-render', function() { ujs.unmountComponents() }); + ujs.handleEvent('DOMContentLoaded', function() { ujs.mountComponents() }) + ujs.handleEvent('turbolinks:render', function() { ujs.mountComponents() }) + ujs.handleEvent('turbolinks:before-render', function() { ujs.unmountComponents() }) }, } diff --git a/test/test_helper.rb b/test/test_helper.rb index 3e81a275..69124073 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -32,7 +32,7 @@ # `page.driver.debug` will cause Poltergeist to open a browser window inspector: true, # hide warnings from React.js whitespace changes: - js_errors: false, + # js_errors: false, } Capybara::Poltergeist::Driver.new(app, poltergeist_options) end From 7141010227439a23985316a867e7180f7bb503b7 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Wed, 12 Apr 2017 09:22:15 -0400 Subject: [PATCH 2/2] Make sure UJS is only ever loaded once --- test/dummy/app/assets/config/manifest.js | 1 + .../app/assets/javascripts/turbolinks_only.js | 1 + .../controllers/pack_components_controller.rb | 2 + .../app/views/layouts/application.html.erb | 5 +- test/react/rails/react_rails_ujs_test.rb | 69 ++++++++++++------- test/react/rails/webpacker_test.rb | 1 + test/test_helper.rb | 2 +- 7 files changed, 56 insertions(+), 25 deletions(-) create mode 100644 test/dummy/app/assets/javascripts/turbolinks_only.js diff --git a/test/dummy/app/assets/config/manifest.js b/test/dummy/app/assets/config/manifest.js index 4265e5b7..ea902f23 100644 --- a/test/dummy/app/assets/config/manifest.js +++ b/test/dummy/app/assets/config/manifest.js @@ -1,4 +1,5 @@ // Sprockets 4 expects this file // //= link application.js +//= link turbolinks_only.js //= link application.css diff --git a/test/dummy/app/assets/javascripts/turbolinks_only.js b/test/dummy/app/assets/javascripts/turbolinks_only.js new file mode 100644 index 00000000..2bd01eb8 --- /dev/null +++ b/test/dummy/app/assets/javascripts/turbolinks_only.js @@ -0,0 +1 @@ +//= require turbolinks diff --git a/test/dummy/app/controllers/pack_components_controller.rb b/test/dummy/app/controllers/pack_components_controller.rb index 20dd2dbc..547f40c2 100644 --- a/test/dummy/app/controllers/pack_components_controller.rb +++ b/test/dummy/app/controllers/pack_components_controller.rb @@ -1,4 +1,6 @@ class PackComponentsController < ApplicationController + # make sure Sprockets application.js isn't loaded: + layout false def show end end diff --git a/test/dummy/app/views/layouts/application.html.erb b/test/dummy/app/views/layouts/application.html.erb index 143fc1fb..3da494ca 100644 --- a/test/dummy/app/views/layouts/application.html.erb +++ b/test/dummy/app/views/layouts/application.html.erb @@ -2,7 +2,10 @@ Dummy - <% if SprocketsHelpers.available? %> + <% if WebpackerHelpers.available? %> + <%= javascript_include_tag "turbolinks_only", "data-turbolinks-track" => true %> + <%= javascript_pack_tag "application" %> + <% elsif SprocketsHelpers.available? %> <%= javascript_include_tag "application", "data-turbolinks-track" => true %> <% end %> <%= csrf_meta_tags %> diff --git a/test/react/rails/react_rails_ujs_test.rb b/test/react/rails/react_rails_ujs_test.rb index 26a05d7b..a74b707a 100644 --- a/test/react/rails/react_rails_ujs_test.rb +++ b/test/react/rails/react_rails_ujs_test.rb @@ -4,14 +4,37 @@ class ReactRailsUJSTest < ActionDispatch::IntegrationTest include Capybara::DSL + compiled = false setup do - Capybara.current_driver = Capybara.javascript_driver - WebpackerHelpers.compile_if_missing + if !compiled + React::ServerRendering.reset_pool + WebpackerHelpers.compile + end + end + + # Normalize for webpacker check: + def assert_greeting(page, plain_greeting, refute: false) + normalized_greeting = if WebpackerHelpers.available? + greeting, name = plain_greeting.split(" ") + "#{greeting} from Webpacker #{name}" + else + plain_greeting + end + + if refute + assert page.has_no_content?(normalized_greeting), page.body + else + assert page.has_content?(normalized_greeting), page.body + end + end + + def refute_greeting(page, greeting) + assert_greeting(page, greeting, refute: true) end test 'ujs object present on the global React object and has our methods' do visit '/pages/1' - assert page.has_content?('Hello Bob') + assert_greeting(page, 'Hello Bob') # the exposed ujs object is present ujs_present = page.evaluate_script('typeof ReactRailsUJS === "object";') @@ -34,82 +57,82 @@ class ReactRailsUJSTest < ActionDispatch::IntegrationTest test 'react_ujs works with rendered HTML' do visit '/pages/1' - assert page.has_content?('Hello Bob') + assert_greeting(page, 'Hello Bob') page.click_button 'Goodbye' - assert page.has_no_content?('Hello Bob') - assert page.has_content?('Goodbye Bob') + refute_greeting(page, 'Hello Bob') + assert_greeting(page, 'Goodbye Bob') end test 'react_ujs works with Turbolinks' do visit '/pages/1' - assert page.has_content?('Hello Bob') + assert_greeting(page, 'Hello Bob') assert page.evaluate_script("Turbolinks.supported") # Try clicking links. page.click_link('Alice') wait_for_turbolinks_to_be_available - assert page.has_content?('Hello Alice') + assert_greeting(page, 'Hello Alice') page.click_link('Bob') wait_for_turbolinks_to_be_available - assert page.has_content?('Hello Bob') + assert_greeting(page, 'Hello Bob') # Try going back. page.execute_script('history.back();') wait_for_turbolinks_to_be_available - assert page.has_content?('Hello Alice') + assert_greeting(page, 'Hello Alice') # Try Turbolinks javascript API. page.execute_script('Turbolinks.visit("/pages/2");') wait_for_turbolinks_to_be_available - assert page.has_content?('Hello Alice') + assert_greeting(page, 'Hello Alice') page.execute_script('Turbolinks.visit("/pages/1");') wait_for_turbolinks_to_be_available - assert page.has_content?('Hello Bob') + assert_greeting(page, 'Hello Bob') # Component state is not persistent after clicking current page link. page.click_button 'Goodbye' - assert page.has_content?('Goodbye Bob') + assert_greeting(page, 'Goodbye Bob') page.click_link('Bob') wait_for_turbolinks_to_be_available - assert page.has_content?('Hello Bob') + assert_greeting(page, 'Hello Bob') end test 'react_ujs can unmount/mount using a selector reference for a component parent' do visit '/pages/1' - assert page.has_content?('Hello Bob'), page.body + assert_greeting(page, 'Hello Bob') page.click_button "Unmount by parent selector" - assert page.has_no_content?('Hello Bob'), page.body + refute_greeting(page, 'Hello Bob') page.click_button "Mount by parent selector" - assert page.has_content?('Hello Bob'), page.body + assert_greeting(page, 'Hello Bob') end test 'react_ujs can unmount/mount using a selector reference for the component' do visit '/pages/1' - assert page.has_content?('Hello Bob'), page.body + assert_greeting(page, 'Hello Bob') page.click_button "Unmount by own selector" - assert page.has_no_content?('Hello Bob'), page.body + refute_greeting(page, 'Hello Bob') page.click_button "Mount by own selector" - assert page.has_content?('Hello Bob'), page.body + assert_greeting(page, 'Hello Bob') end test 'react_ujs can unmount/mount using a dom node context' do visit '/pages/1' - assert page.has_content?('Hello Bob'), page.body + assert_greeting(page, 'Hello Bob') page.click_button "Unmount by parent node" - assert page.has_no_content?('Hello Bob'), page.body + refute_greeting(page, 'Hello Bob') page.click_button "Mount by parent node" - assert page.has_content?('Hello Bob'), page.body + assert_greeting(page, 'Hello Bob') end test 'react server rendering also gets mounted on client' do diff --git a/test/react/rails/webpacker_test.rb b/test/react/rails/webpacker_test.rb index 5b54887f..15e44077 100644 --- a/test/react/rails/webpacker_test.rb +++ b/test/react/rails/webpacker_test.rb @@ -7,6 +7,7 @@ class ReactRailsWebpackerTest < ActionDispatch::IntegrationTest setup do Capybara.current_driver = Capybara.javascript_driver WebpackerHelpers.compile + React::ServerRendering.reset_pool end teardown do diff --git a/test/test_helper.rb b/test/test_helper.rb index 69124073..2d9e3e39 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -24,7 +24,6 @@ WebpackerHelpers.clear_webpacker_packs -Capybara.javascript_driver = :poltergeist Capybara.app = Rails.application Capybara.register_driver :poltergeist_debug do |app| @@ -37,6 +36,7 @@ Capybara::Poltergeist::Driver.new(app, poltergeist_options) end Capybara.javascript_driver = :poltergeist_debug +Capybara.current_driver = Capybara.javascript_driver CACHE_PATH = Pathname.new File.expand_path("../dummy/tmp/cache", __FILE__)