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

[vegetabill]

is not used.

• allow user to override the components_js lamdba

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[vipulnsward]

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

[vegetabill]

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

[vipulnsward]

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,

[vegetabill]

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?

[vipulnsward]

@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

[vipulnsward]

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

[rmosolgo]

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?

[vegetabill]

@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?

[johnthethird]

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.

[vipulnsward]

extra space

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[danhart]

This fix also appears to address the same issue: master...ownerscircle:perf_testing

Not sure which one is preferable. We used master...ownerscircle:perf_testing in our project, rather than this one.

[vegetabill]

@danhart thanks for sharing your code. Personally, I feel your solution is superior. I'd suggest opening a PR for your change and referencing it as a solution to #212. It definitely solves the original problem in a clean way and I hope it would be merged.

However, since I opened this PR, the app I'm working on is now in a transition period from a sprocket-based asset pipeline to a fully gulp one, and in the meantime we have both running simultaneously. In the spirit of making this gem flexible and configurable and not forcing client apps to fork the repo—even if they're doing something a little crazy like I am, I think it would still be worth merging this change, unless there's some downside to having the components_js lambda overridable?

@johnthethird or others who might know: was there a reason this PR was never merged? I'd really like it to be merged so I can stop using my fork. I see the travis build failed but it doesn't seem related to my change. If this PR would be mergeable assuming a green build, I'd be happy to investigate further so we could close this out.

[rmosolgo]

At first I didn't merge this because it had this "unresolved question" of threadsafety. (I think it's been resolved: "NBD.") But now it doesn't merge because server rendering has been totally refactored.

Besides that, I try to avoid "just make it a setting", but I didn't have a better suggestion at the time.

Now, this feature is supported by passing options to config.react.server_renderer_options. For example:

 config.react.server_renderer_options = {
    files: ["react.js", "components.js"], # files to load for prerendering
    replay_console: true,                 # if true, console.* will be replayed client-side
  }

You could use whatever you want for files:.

Would that work for your use case?

[vegetabill]

I don't think those options alone will allow users to solve this issue. It's not which files to read, but how they are read. Looking at https://github.com/reactjs/react-rails/blob/master/lib/react/server_rendering/sprockets_renderer.rb#L13 it still seems to be hardcoded to assume the asset pipeline exists in production, which is actually not the best practice for Rails. Because of that I think it would be ideal if you incorporated the relevant part (master...ownerscircle:perf_testing#diff-1cd07a77af52dfe22d04099826f272a9R20) of @danhart 's fix into sprockets_renderer so it will work whether the asset compilation pipeline is enabled or not.

But as for my weird use case, with your awesome refactoring I can now just make my own subclass of the renderer to do whatever I want now so I'm going to close this PR.