Not pusing mini_racer in default install. Closes #1438 #1437

vtamara · 2022-03-31T17:17:33Z

node is used by default and usually it has less problems than mini_racer.

react_on_rails shouldn't push mini_racer in new installations. This patch removes such behavior.

This change is

… it has less problems than mini_racer

justin808 · 2022-05-13T08:06:23Z

@alexeyr @tomdracz Please take a look at this.

Is this a breaking change in any way?
Recommended under what circumstances?
Any downsides?

I don't quite understand why recently this change is needed.

tomdracz · 2022-05-13T08:59:41Z

From my perspective:

agree that mini_racer creates some headaches. Had a lot of time spent debugging and swearing when CI wasn't playing ball with libv8 versions
Breaking change? Possibly, maybe. You might hit some implementation differences but considering Node itself is built on top of v8, I wouldn't expect much difference apart from some edge cases
Having mini_racer as dependency means sneakily changing ExecJS runtime from Node to mini_racer if it wasn't previously specified. Following principle of least surprise, I would rather have the app dictate what they use for the JS engine. If people want to use mini_racer for whatever circumstances, we should let them, but they should do it in the main app Gemfile.
Can't see downsides here really. Considering you will need Node either way and it's faster, I see no reason to rely on mini_racer
mini_racer is only useful if you want to use your app in an environment without existing JS engine. But you will need Node to compile assets so this point is moot for anyone using react_on_rails

I've ran a few tests on my app that is using mini_racer gem - yanking it out resulted in no visible changes, app still works, test suite still passes etc.

Happy for this one to go, will add some minor comments.

tomdracz · 2022-05-13T08:59:41Z

From my perspective:

agree that mini_racer creates some headaches. Had a lot of time spent debugging and swearing when CI wasn't playing ball with libv8 versions
Breaking change? Possibly, maybe. You might hit some implementation differences but considering Node itself is built on top of v8, I wouldn't expect much difference apart from some edge cases
Having mini_racer as dependency means sneakily changing ExecJS runtime from Node to mini_racer if it wasn't previously specified. Following principle of least surprise, I would rather have the app dictate what they use for the JS engine. If people want to use mini_racer for whatever circumstances, we should let them, but they should do it in the main app Gemfile.
Can't see downsides here really. Considering you will need Node either way and it's faster, I see no reason to rely on mini_racer
mini_racer is only useful if you want to use your app in an environment without existing JS engine. But you will need Node to compile assets so this point is moot for anyone using react_on_rails

I've ran a few tests on my app that is using mini_racer gem - yanking it out resulted in no visible changes, app still works, test suite still passes etc.

Happy for this one to go, will add some minor comments.

tomdracz · 2022-05-13T09:01:10Z

docs/guides/client-vs-server-rendering.md

@@ -4,7 +4,7 @@

 In most cases, you should use the `prerender: false` (default behavior) with the provided helper method to render the React component from your Rails views. In some cases, such as when SEO is vital, or many users will not have JavaScript enabled, you can enable server-rendering by passing `prerender: true` to your helper, or you can simply change the default in `config/initializers/react_on_rails`.

-Now the server will interpret your JavaScript. The default is to use [ExecJS](https://github.com/rails/execjs) and pass the resulting HTML to the client. We recommend using [mini_racer](https://github.com/discourse/mini_racer) as ExecJS's runtime.
+Now the server will interpret your JavaScript. The default is to use Node.js.


Think ExecJS will still be used behind the scenes but with Node.js rather than mini_racer. Maybe change this to reflect this and tell people they've got choice of alternative runtimes, as documented in ExecJS readme?

That would be my suggestion too.

Actually, can anything other than ExecJS be used? If not, the correct phrasing could be something like "Now the server will interpret your JavaScript using ExecJS. You can pick the engine as described in the documentation." If yes, this is a good place to explain how.

tomdracz · 2022-05-13T09:01:38Z

docs/guides/configuration.md

@@ -137,14 +137,14 @@ ReactOnRails.configure do |config|
  ################################################################################
  # Server Renderer Configuration for ExecJS
  ################################################################################
-  # The default server rendering is ExecJS, probably using the mini_racer gem
+  # The default server rendering is Node.js


As above, still will be ExecJS but with Node runtime

Yes, but since it already says "for ExecJS" just 2 lines above I don't know if it needs to be said again.

tomdracz · 2022-05-13T09:01:38Z

docs/guides/configuration.md

@@ -137,14 +137,14 @@ ReactOnRails.configure do |config|
  ################################################################################
  # Server Renderer Configuration for ExecJS
  ################################################################################
-  # The default server rendering is ExecJS, probably using the mini_racer gem
+  # The default server rendering is Node.js


As above, still will be ExecJS but with Node runtime

tomdracz · 2022-05-13T09:03:02Z

docs/guides/configuration.md

  # If you wish to use an alternative Node server rendering for higher performance,
  # contact justin@shakacode.com for details.
  #
  # For ExecJS:
  # You can configure your pool of JS virtual machines and specify where it should load code:
-  # On MRI, use `mini_racer` for the best performance
-  # (see [discussion](https://github.com/reactjs/react-rails/pull/290))


The discussion there has shown that mini_racer performed better than Node. That was quite a few years back, would be good to link to any benchmarks supporting the fact that Node is more peformant

tomdracz · 2022-05-13T09:03:02Z

docs/guides/configuration.md

  # If you wish to use an alternative Node server rendering for higher performance,
  # contact justin@shakacode.com for details.
  #
  # For ExecJS:
  # You can configure your pool of JS virtual machines and specify where it should load code:
-  # On MRI, use `mini_racer` for the best performance
-  # (see [discussion](https://github.com/reactjs/react-rails/pull/290))


The discussion there has shown that mini_racer performed better than Node. That was quite a few years back, would be good to link to any benchmarks supporting the fact that Node is more peformant

tomdracz · 2022-05-13T09:04:08Z

lib/generators/react_on_rails/base_generator.rb

@@ -73,7 +73,6 @@ def copy_webpacker_config
      end

      def add_base_gems_to_gemfile
-        gem "mini_racer", platforms: :ruby


We should have a changelog entry reflecting that. If users already have mini_racer in their Gemfiles, nothing will change. We should note that it's been removed and ask users to also remove and test their apps for any changes

tomdracz · 2022-05-13T09:04:08Z

lib/generators/react_on_rails/base_generator.rb

@@ -73,7 +73,6 @@ def copy_webpacker_config
      end

      def add_base_gems_to_gemfile
-        gem "mini_racer", platforms: :ruby


We should have a changelog entry reflecting that. If users already have mini_racer in their Gemfiles, nothing will change. We should note that it's been removed and ask users to also remove and test their apps for any changes

vtamara added 2 commits March 31, 2022 11:41

mini_racer not needed by default. node is used by default and usually…

e95002f

… it has less problems than mini_racer

Not suggesting mini_racer

e10b3f5

vtamara force-pushed the no-miniracer branch 2 times, most recently from 8245fac to e10b3f5 Compare March 31, 2022 22:36

vtamara changed the title ~~mini_racer not needed by default.~~ Not pusing mini_racer in default install. Closes #1438 Mar 31, 2022

alexeyr-c mentioned this pull request Apr 20, 2022

RSpec example tests fail under mini_racer #1447

Closed

tomdracz suggested changes May 13, 2022

View reviewed changes

tomdracz mentioned this pull request May 13, 2022

Remove mini_racer as the default ExecJS runtime #1453

Merged

justin808 closed this in #1453 Jun 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Not pusing mini_racer in default install. Closes #1438 #1437

Not pusing mini_racer in default install. Closes #1438 #1437

vtamara commented Mar 31, 2022 •

edited by justin808

Loading

justin808 commented May 13, 2022

tomdracz commented May 13, 2022

tomdracz commented May 13, 2022

tomdracz May 13, 2022

alexeyr-c May 13, 2022

alexeyr-c May 13, 2022

tomdracz May 13, 2022

alexeyr-c May 13, 2022

tomdracz May 13, 2022

tomdracz May 13, 2022

tomdracz May 13, 2022

tomdracz May 13, 2022

tomdracz May 13, 2022

Not pusing mini_racer in default install. Closes #1438 #1437

Not pusing mini_racer in default install. Closes #1438 #1437

Conversation

vtamara commented Mar 31, 2022 • edited by justin808 Loading

justin808 commented May 13, 2022

tomdracz commented May 13, 2022

tomdracz commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtamara commented Mar 31, 2022 •

edited by justin808

Loading