Skip to content
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

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e95002f
mini_racer not needed by default. node is used by default and usually…
vtamara Mar 31, 2022
e10b3f5
Not suggesting mini_racer
vtamara Mar 31, 2022
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
2 changes: 0 additions & 2 deletions Gemfile.development_dependencies
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ gem "sprockets", "~> 4.0"

gem "amazing_print"

gem "mini_racer", ">= 0.6.2"

group :development, :test do
gem "listen"
gem "pry"
Expand Down
2 changes: 1 addition & 1 deletion docs/guides/client-vs-server-rendering.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

That would be my suggestion too.

Copy link

Choose a reason for hiding this comment

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

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.


If you want to maximize the perfomance of your server rendering, then you want to use React on Rails Pro which uses NodeJS to do the server rendering. See the [docs for React on Rails Pro](https://github.com/shakacode/react_on_rails/wiki).

Expand Down
6 changes: 3 additions & 3 deletions docs/guides/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, still will be ExecJS but with Node runtime

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

As above, still will be ExecJS but with Node runtime

# 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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

# On MRI, use `Node.js` for good performance
# (see [issue](https://github.com/shakacode/react_on_rails/issues/1438))
# 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.
config.server_renderer_pool_size = 1 # increase if you're on JRuby
Expand Down
2 changes: 0 additions & 2 deletions docs/guides/tutorial.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,6 @@ Install React on Rails: `rails generate react_on_rails:install`. You need to fir
Note, using `redux` is no longer recommended as the basic installer uses React Hooks.
If you want the redux install: `rails generate react_on_rails:install --redux`

The generator will add `mini_racer`'s latest version. If you're using linux & encounter issues installing `libv8`, here's [a common solution](https://github.com/rubyjs/mini_racer/issues/218).

```
bundle exec rails generate react_on_rails:install
```
Expand Down
3 changes: 0 additions & 3 deletions docs/javascript/server-rendering-tips.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@

For the best performance with Server Rendering, consider using [React on Rails Pro]

Be sure to use mini_racer. See [issues/428](https://github.com/shakacode/react_on_rails/issues/428).



## General Tips
- Your code can't reference `document`. Server side JS execution does not have access to `document`,
Expand Down
1 change: 0 additions & 1 deletion lib/generators/react_on_rails/base_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ def copy_webpacker_config
end

def add_base_gems_to_gemfile
gem "mini_racer", platforms: :ruby
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

run "bundle"
end

Expand Down