Skip to content

Commit

Permalink
added check for development to reload server bundle ever time it chan…
Browse files Browse the repository at this point in the history
…ges. changed links in readme from shell to markdown links. moved linters order
  • Loading branch information
jbhatab committed Nov 1, 2015
1 parent 2c0d6c5 commit 66b7408
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 6 deletions.
15 changes: 15 additions & 0 deletions app/helpers/react_on_rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,11 @@ def next_react_component_index
def server_rendered_react_component_html(options, props_string, react_component_name, data_variable_name, dom_id)
return ["", ""] unless prerender(options)

# Check if server bundle timestamp has changed.
# If it has, then clear the connection pool.
# Only for development.
reload_server_bundle_if_modified

wrapper_js = <<-JS
(function() {
var props = #{props_string};
Expand All @@ -158,6 +163,16 @@ def server_rendered_react_component_html(options, props_string, react_component_
raise ReactOnRails::ServerRenderingPool::PrerenderError.new(react_component_name, props_string, err)
end

def reload_server_bundle_if_modified
return unless ReactOnRails.configuration.reload_server_js_every_request
file_mtime = File.mtime(ReactOnRails.configuration.server_bundle_js_file)
@@server_bundle_timestamp ||= file_mtime
if @@server_bundle_timestamp != file_mtime
ReactOnRails::ServerRenderingPool.reset_pool
@@server_bundle_timestamp = file_mtime
end
end

This comment has been minimized.

Copy link
@justin808

justin808 Nov 1, 2015

Member

I think this should go one level lower.

justin [12:38 AM]
this should go into the server rendering pool

justin [12:38 AM]

  • def reload_server_bundle_if_modified
  • return unless ReactOnRails.configuration.reload_server_js_every_request
  • file_mtime = File.mtime(ReactOnRails.configuration.server_bundle_js_file)
  • @@server_bundle_timestamp ||= file_mtime
  • if @@server_bundle_timestamp != file_mtime
  •  ReactOnRails::ServerRenderingPool.reset_pool
    
  •  @@server_bundle_timestamp = file_mtime
    
  • end

justin [12:39 AM]
looks good otherwise

justin [12:39 AM]
I really don’t like just slapping a @@ var on the helper

it just feels like a bit of a messy use of a global

def trace(options)
options.fetch(:trace) { ReactOnRails.configuration.trace }
end
Expand Down
2 changes: 1 addition & 1 deletion lib/generators/react_on_rails/linters_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ class LintersGenerator < Rails::Generators::Base
def add_linter_gems
gem_group :development do
gem("rubocop", require: false)
gem("scss_lint", require: false)
gem("ruby-lint", require: false)
gem("scss_lint", require: false)
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ The controller is located at `app/controllers/hello_world_controller.rb`
See [the documentation](https://github.com/shakacode/react_on_rails) for how to build your bundles and
install your packages. Then you can view the example as follows:

-"Hello World" example: `localhost:3000/hello_world`
-"Hello World" example: [localhost:3000/hello_world](localhost:3000/hello_world)
-"Hello World" example via Webpack Development Server with Hot Module Reload: `localhost:4000`
<%- end %>

Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
config.prerender = false # default is false
config.generator_function = false # default is false, meaning that you expose ReactComponents directly
config.trace = Rails.env.development? # default is true for development, off otherwise
config.reload_server_js_every_request = Rails.env.development? # Forces server loaded bundle to reload every js request

# For server rendering. This can be set to false so that server side messages are discarded.
config.replay_console = true # Default is true. Be cautious about turning this off.
Expand Down
9 changes: 6 additions & 3 deletions lib/react_on_rails/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,19 @@ def self.configuration
logging_on_server: true,
generator_function: false,
trace: Rails.env.development?,
reload_server_js_every_request: Rails.env.development?,
server_renderer_pool_size: 1,
server_renderer_timeout: 20)
end

class Configuration
attr_accessor :server_bundle_js_file, :prerender, :replay_console, :generator_function, :trace,
:logging_on_server, :server_renderer_pool_size, :server_renderer_timeout
:reload_server_js_every_request, :logging_on_server, :server_renderer_pool_size, :server_renderer_timeout

def initialize(server_bundle_js_file: nil, prerender: nil, replay_console: nil,
generator_function: nil, trace: nil, logging_on_server: nil,
server_renderer_pool_size: nil, server_renderer_timeout: nil)
generator_function: nil, trace: nil, reload_server_js_every_request: nil,
logging_on_server: nil, server_renderer_pool_size: nil,
server_renderer_timeout: nil)
if File.exist?(server_bundle_js_file)
self.server_bundle_js_file = server_bundle_js_file
else
Expand All @@ -32,6 +34,7 @@ def initialize(server_bundle_js_file: nil, prerender: nil, replay_console: nil,
self.replay_console = replay_console
self.logging_on_server = logging_on_server
self.generator_function = generator_function
self.reload_server_js_every_request = reload_server_js_every_request.nil? ? Rails.env.development? : reload_server_js_every_request

This comment has been minimized.

Copy link
@justin808

justin808 Nov 1, 2015

Member

we need to force the renderer pool size to be 1 if the reload_server_ever_js_request is true.

Otherwise, we could have multiple contexts. I'm guessing that would be an issue. Would it?

self.trace = trace.nil? ? Rails.env.development? : trace

# Server rendering:
Expand Down
1 change: 0 additions & 1 deletion lib/react_on_rails/server_rendering_pool.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ def trace_messsage(js_code)

def eval_js(js_code)
@js_context_pool.with do |js_context|
# binding.pry
result = js_context.eval(js_code)
js_context.eval("console.history = []")
result
Expand Down

3 comments on commit 66b7408

@justin808
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one important suggestion regarding where the logic goes on reloading the server code.

@justin808
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we had a test for this. If we had a test that had compiled even the simplest function that returned a small string, we could change that js code to return a different value, and verify this is working. You'd also need to config the gem preference, as you'd be in test mode.

@jbhatab
Copy link
Member Author

@jbhatab jbhatab commented on 66b7408 Nov 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree. I can add that later.

Please sign in to comment.