Skip to content

Fix for Issue #212 components.js generated by assets:precompile #213

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix for Issue #212 components.js generated by assets:precompile #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,9 +316,21 @@ MyApp::Application.configure do
config.react.react_js = lambda {File.read(::Rails.application.assets.resolve('react.js'))}
config.react.component_filenames = ['components.js']
end
```

#### Precompiled Component File

The default configuration assumes the asset pipeline is available in all environments. If you precompile assets for production and want to fetch `components.js` from the precompiled assets, add this additional configuration:

```ruby
config.react.component_js = lambda do
config.react.component_filenames.map do |filename|
File.read(Rails.root.join('public', 'assets', filename))
end.join(';')
end
```


Copy link
Contributor

Choose a reason for hiding this comment

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

extra space

## CoffeeScript

It is possible to use JSX with CoffeeScript. The caveat is that you will still need to include the docblock. Since CoffeeScript doesn't allow `/* */` style comments, we need to do something a little different. We also need to embed JSX inside backticks so CoffeeScript ignores the syntax it doesn't understand. Here's an example:
Expand Down
2 changes: 1 addition & 1 deletion lib/react/rails/railtie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class Railtie < ::Rails::Railtie
config.after_initialize do |app|
# Server Rendering
# Concat component_filenames together for server rendering
app.config.react.components_js = lambda {
app.config.react.components_js ||= lambda {
Copy link
Contributor

Choose a reason for hiding this comment

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

||= is not threadsafe, you can add an if check for presence of the value instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A naive if would probably suffer from a possible race condition as well. Does multithreaded Rails not have single-threaded initialization?

Copy link
Contributor

Choose a reason for hiding this comment

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

A naive if would probably suffer from a possible race condition as well.

I din't get you, how over here?

Does multithreaded Rails not have single-threaded initialization?

No. As a rule of thumb, its better to avoid ||= in places, where multithreaded execution is expected, something like here. Else it will fail sporadically,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still confused as to how a plain if will be any more or less threadsafe than ||=. Can you show me how you would write this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

@RearAdmiral x ||= 10, expands to x = x || 10 . This is not guarded, and all three expressions in the latter are sequential, so x is not always guaranteed to be 10 here by ruby.
An if, wouldn't be affected by this. You could simple do

if app.config.react.components_js.nil?
  app.config.react.components_js = ...
  ...
end

Copy link
Contributor

Choose a reason for hiding this comment

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

@RearAdmiral I am not sure, how else I could explain this 😄

Copy link
Member

Choose a reason for hiding this comment

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

I don't know much about threadsafety and Rails, but I thought I'd see if other gems do the same or not. I found a couple examples:

https://github.com/rails/sass-rails/blob/master/lib/sass/rails/railtie.rb#L69-L74

https://github.com/rails/rails/blob/master/activejob/lib/active_job/railtie.rb#L14-L15

(I didn't find any ||= in ActiveRecord, Carrierwave, Paperclip or Sidekiq railties)

Is there a reason those are different than our case? If it works for them, can we expect it to work for us? If not, does anyone threadsafe suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vipulnsward It would be much easier with a whiteboard :-) but I would appreciate if you'd explain more because I wasn't aware of this problem and would like to learn more.

x is not always guaranteed to be 10 here by ruby

What are the possible values then? I assumed it would either remain unchanged or be set to 10.

I still don't think either implementation is threadsafe. Rails.app.config is global so in theory another thread could set it to nil and break either case.

For example,if our react-rails init thread has just checked if app.config.react.components_js is nil either by your .nil? guard or the ||= statement, and some other thread sets it to nil at that exact moment, we won't assign it any value and it will remain nil despite it looking like our intention was to assign it a default value when nil.

So are you specifically referring to this code being run simultaneously by multiple threads and leading to unexpected outcomes? By my reading, the unexpected outcome would be the creation of two lambdas which is wasteful but wouldn't break any functionality. Can you explain a case where it would break?

Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity, there is no difference between an ||= or an if as far as thread safety. If you want to insure a set of instructions is run uninterrupted you have to use a mutex of some kind. Period. Also, when Rails is initializing, you dont need to worry about threading issues. Those only happen when multiple requests come in to (certain) servers (Puma, etc) which spawn threads to handle each request. By that time the app has already initialized.

app.config.react.component_filenames.map do |filename|
app.assets[filename].to_s
end.join(";")
Expand Down