Merge pull request #302 from rmosolgo/transformer-fixes

Config & initialization fixes

Author: Robert Mosolgo

README.md

@@ -188,7 +188,7 @@ You can configure your pool of JS virtual machines and specify where it should l
 # These are the defaults if you dont specify any yourself
 MyApp::Application.configure do
   # Settings for the pool of renderers:
-  config.react.server_renderer_pool_size  ||= 10
+  config.react.server_renderer_pool_size  ||= 1  # ExecJS doesn't allow more than one on MRI
   config.react.server_renderer_timeout    ||= 20 # seconds
   config.react.server_renderer = React::ServerRendering::SprocketsRenderer
   config.react.server_renderer_options = {
@@ -198,6 +198,10 @@ MyApp::Application.configure do
 end
 ```
 
+- On MRI, use `therubyracer` for the best performance (see [discussion](https://github.com/reactjs/react-rails/pull/290))
+- On MRI, you'll get a deadlock with `pool_size` > 1
+- If you're using JRuby, you can increase `pool_size` to have real multi-threaded rendering.
+
 ### Component generator
 
 `react-rails` ships with a Rails generator to help you get started with a simple component scaffold. 

lib/react/jsx.rb

@@ -6,24 +6,18 @@
 
 module React
   module JSX
-    mattr_accessor :transform_options, :transformer_class
+    DEFAULT_TRANSFORMER = BabelTransformer
+    mattr_accessor :transform_options, :transformer_class, :transformer
 
     # You can assign `React::JSX.transformer_class = `
     # to provide your own transformer. It must implement:
     # - #initialize(options)
     # - #transform(code) => new code
-    self.transformer_class = BabelTransformer
+    self.transformer_class = DEFAULT_TRANSFORMER
 
     def self.transform(code)
-      transformer.transform(code)
-    end
-
-    def self.transformer
-      # lazily loaded during first request and reloaded every time when in dev or test
-      if @transformer.nil? || !::Rails.env.production?
-        @transformer = transformer_class.new(transform_options)
-      end
-      @transformer
+      self.transformer ||= transformer_class.new(transform_options)
+      self.transformer.transform(code)
     end
   end
 end

lib/react/rails.rb

@@ -1,3 +1,4 @@
-require 'react/rails/railtie'
+require 'react/rails/asset_variant'
 require 'react/rails/engine'
+require 'react/rails/railtie'
 require 'react/rails/view_helper'

lib/react/rails/asset_variant.rb

@@ -0,0 +1,19 @@
+module React
+  module Rails
+    class AssetVariant
+      GEM_ROOT = Pathname.new('../../../../').expand_path(__FILE__)
+      attr_reader :react_build, :react_directory, :jsx_directory
+
+      def initialize(options={})
+        # We want to include different files in dev/prod. The development builds
+        # contain console logging for invariants and logging to help catch
+        # common mistakes. These are all stripped out in the production build.
+        @react_build = options[:variant] == :production ? 'production' : 'development'
+        options[:addons] && @react_build += '-with-addons'
+
+        @react_directory = GEM_ROOT.join('lib/assets/react-source/').join(@react_build).to_s
+        @jsx_directory =   GEM_ROOT.join('lib/assets/javascripts/').to_s
+      end
+    end
+  end
+end

lib/react/rails/railtie.rb

@@ -9,9 +9,10 @@ class Railtie < ::Rails::Railtie
       config.react.variant = (::Rails.env.production? ? :production : :development)
       config.react.addons = false
       config.react.jsx_transform_options = {}
+      config.react.jsx_transformer_class = nil # defaults to BabelTransformer
       # Server rendering:
-      config.react.server_renderer_pool_size  = 10
-      config.react.server_renderer_timeout    = 20 # seconds
+      config.react.server_renderer_pool_size  = 1   # increase if you're on JRuby
+      config.react.server_renderer_timeout    = 20  # seconds
       config.react.server_renderer            = nil # defaults to SprocketsRenderer
       config.react.server_renderer_options    = {}  # SprocketsRenderer provides defaults
 
@@ -22,32 +23,35 @@ class Railtie < ::Rails::Railtie
 
       # Include the react-rails view helper lazily
       initializer "react_rails.setup_view_helpers", group: :all do |app|
+        app.config.react.jsx_transformer_class ||= React::JSX::DEFAULT_TRANSFORMER
+        React::JSX.transformer_class = app.config.react.jsx_transformer_class
         React::JSX.transform_options = app.config.react.jsx_transform_options
+
         ActiveSupport.on_load(:action_view) do
           include ::React::Rails::ViewHelper
         end
       end
 
       initializer "react_rails.bust_cache", group: :all do |app|
-        variant = app.config.react.variant == :production ? 'production' : 'development'
-        variant += '-with-addons' if app.config.react.addons
+        asset_variant = React::Rails::AssetVariant.new({
+          variant: app.config.react.variant,
+          addons: app.config.react.addons,
+        })
 
         app.assets.version = [
           app.assets.version,
-          "react-#{variant}",
+          "react-#{asset_variant.react_build}",
         ].compact.join('-')
       end
 
       config.before_initialize do |app|
-        # We want to include different files in dev/prod. The development builds
-        # contain console logging for invariants and logging to help catch
-        # common mistakes. These are all stripped out in the production build.
-        root_path = Pathname.new('../../../../').expand_path(__FILE__)
-        directory = app.config.react.variant == :production ? 'production' : 'development'
-        directory += '-with-addons' if app.config.react.addons
+        asset_variant = React::Rails::AssetVariant.new({
+          variant: app.config.react.variant,
+          addons: app.config.react.addons,
+        })
 
-        app.config.assets.paths << root_path.join('lib/assets/react-source/').join(directory).to_s
-        app.config.assets.paths << root_path.join('lib/assets/javascripts/').to_s
+        app.config.assets.paths << asset_variant.react_directory
+        app.config.assets.paths << asset_variant.jsx_directory
       end
 
       config.after_initialize do |app|

test/react/jsx/jsx_transformer_test.rb

@@ -0,0 +1,58 @@
+require 'test_helper'
+
+class JSXTransformerTest < ActionDispatch::IntegrationTest
+  setup do
+    reset_transformer
+    React::JSX.transformer_class = React::JSX::JSXTransformer
+  end
+
+  teardown do
+    reset_transformer
+  end
+
+  test 'can use dropped-in version of JSX transformer' do
+    hidden_path =     Rails.root.join("vendor/assets/react/JSXTransformer__.js")
+    replacing_path =  Rails.root.join("vendor/assets/react/JSXTransformer.js")
+
+    FileUtils.cp hidden_path, replacing_path
+    get '/assets/example3.js'
+    FileUtils.rm replacing_path
+
+    assert_response :success
+    assert_equal 'test_confirmation_token_jsx_transformed;', @response.body
+  end
+
+  test 'accepts harmony: true option' do
+    React::JSX.transform_options = {harmony: true}
+    get '/assets/harmony_example.js'
+    assert_response :success
+    assert_match(/generateGreeting:\s*function\(\)/, @response.body, "object literal methods")
+    assert_match(/React.__spread/, @response.body, "spreading props")
+    assert_match(/Your greeting is: '" \+ insertedGreeting \+ "'/, @response.body, "string interpolation")
+    assert_match(/active=\$__0\.active/, @response.body, "destructuring assignment")
+  end
+
+  test 'accepts strip_types: true option' do
+    React::JSX.transform_options = {strip_types: true, harmony: true}
+    get '/assets/flow_types_example.js'
+    assert_response :success
+    assert_match(/\(i\s*,\s*name\s*\)\s*\{/, @response.body, "type annotations are removed")
+  end
+
+  test 'accepts asset_path: option' do
+    hidden_path =     Rails.root.join("vendor/assets/react/JSXTransformer__.js")
+    custom_path =     Rails.root.join("vendor/assets/react/custom")
+    replacing_path =  custom_path.join("CustomTransformer.js")
+
+    React::JSX.transform_options = {asset_path: "custom/CustomTransformer.js"}
+
+    FileUtils.mkdir_p(custom_path)
+    FileUtils.cp(hidden_path, replacing_path)
+    get '/assets/example3.js'
+
+    FileUtils.rm_rf custom_path
+    assert_response :success
+    assert_equal 'test_confirmation_token_jsx_transformed;', @response.body
+  end
+
+end

test/react/jsx_test.rb

@@ -36,12 +36,6 @@ class JSXTransformTest < ActionDispatch::IntegrationTest
     reset_transformer
   end
 
-  def reset_transformer
-    clear_sprockets_cache
-    React::JSX.transformer_class = React::JSX::BabelTransformer
-    React::JSX.transform_options = {}
-  end
-
   test 'asset pipeline should transform JSX' do
     get '/assets/example.js'
     assert_response :success
@@ -73,68 +67,4 @@ def test_babel_transformer_accepts_babel_transformation_options
 
     assert !@response.body.include?('strict')
   end
-
-end
-
-class JSXTransformerTest < ActionDispatch::IntegrationTest
-
-  setup do
-    reset_transformer
-  end
-
-  teardown do
-    reset_transformer
-  end
-
-  def reset_transformer
-    clear_sprockets_cache
-    React::JSX.transformer_class = React::JSX::JSXTransformer
-    React::JSX.transform_options = {}
-  end
-
-  test 'can use dropped-in version of JSX transformer' do
-    hidden_path =     Rails.root.join("vendor/assets/react/JSXTransformer__.js")
-    replacing_path =  Rails.root.join("vendor/assets/react/JSXTransformer.js")
-
-    FileUtils.cp hidden_path, replacing_path
-    get '/assets/example3.js'
-    FileUtils.rm replacing_path
-
-    assert_response :success
-    assert_equal 'test_confirmation_token_jsx_transformed;', @response.body
-  end
-
-  test 'accepts harmony: true option' do
-    React::JSX.transform_options = {harmony: true}
-    get '/assets/harmony_example.js'
-    assert_response :success
-    assert_match(/generateGreeting:\s*function\(\)/, @response.body, "object literal methods")
-    assert_match(/React.__spread/, @response.body, "spreading props")
-    assert_match(/Your greeting is: '" \+ insertedGreeting \+ "'/, @response.body, "string interpolation")
-    assert_match(/active=\$__0\.active/, @response.body, "destructuring assignment")
-  end
-
-  test 'accepts strip_types: true option' do
-    React::JSX.transform_options = {strip_types: true, harmony: true}
-    get '/assets/flow_types_example.js'
-    assert_response :success
-    assert_match(/\(i\s*,\s*name\s*\)\s*\{/, @response.body, "type annotations are removed")
-  end
-
-  test 'accepts asset_path: option' do
-    hidden_path =     Rails.root.join("vendor/assets/react/JSXTransformer__.js")
-    custom_path =     Rails.root.join("vendor/assets/react/custom")
-    replacing_path =  custom_path.join("CustomTransformer.js")
-
-    React::JSX.transform_options = {asset_path: "custom/CustomTransformer.js"}
-
-    FileUtils.mkdir_p(custom_path)
-    FileUtils.cp(hidden_path, replacing_path)
-    get '/assets/example3.js'
-
-    FileUtils.rm_rf custom_path
-    assert_response :success
-    assert_equal 'test_confirmation_token_jsx_transformed;', @response.body
-  end
-
 end

test/react/rails/asset_variant_test.rb

@@ -0,0 +1,25 @@
+require 'test_helper'
+
+class AssetVariantTest < ActiveSupport::TestCase
+  def build_variant(options)
+    React::Rails::AssetVariant.new(options)
+  end
+
+  test 'it points to different directories for react' do
+    production_variant = build_variant(variant: :production)
+    assert_match(%r{/lib/assets/react-source/production}, production_variant.react_directory)
+
+    development_variant = build_variant(variant: nil)
+    assert_match(%r{/lib/assets/react-source/development}, development_variant.react_directory)
+  end
+
+  test 'points to jsx transformer' do
+    variant = build_variant({})
+    assert_match(%r{/lib/assets/javascripts/}, variant.jsx_directory)
+  end
+
+  test 'it includes addons if requested' do
+    asset_variant = build_variant(addons: true)
+    assert_equal "development-with-addons", asset_variant.react_build
+  end
+end

test/test_helper.rb

@@ -24,6 +24,13 @@ def clear_sprockets_cache
   end
 end
 
+def reset_transformer
+  clear_sprockets_cache
+  React::JSX.transformer_class = React::JSX::DEFAULT_TRANSFORMER
+  React::JSX.transform_options = {}
+  React::JSX.transformer = nil
+end
+
 # Sprockets 2 doesn't expire this assets well in
 # this kind of setting,
 # so override `fresh?` to mark it as expired.