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.

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

should force load react-components which send over turbo-stream #1620

Merged
merged 5 commits into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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: 2 additions & 0 deletions Gemfile.development_dependencies
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ gem "sprockets", "~> 4.0"

gem "amazing_print"

gem "turbo-rails"

group :development, :test do
gem "package_json"
gem "listen"
Expand Down
7 changes: 6 additions & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
react_on_rails (14.0.3)
react_on_rails (14.0.4)
addressable
connection_pool
execjs (~> 2.5)
Expand Down Expand Up @@ -369,6 +369,10 @@ GEM
tins (1.33.0)
bigdecimal
sync
turbo-rails (2.0.6)
actionpack (>= 6.0.0)
activejob (>= 6.0.0)
railties (>= 6.0.0)
turbolinks (5.2.1)
turbolinks-source (~> 5.2)
turbolinks-source (5.2.0)
Expand Down Expand Up @@ -431,6 +435,7 @@ DEPENDENCIES
spring (~> 4.0)
sprockets (~> 4.0)
sqlite3 (~> 1.6)
turbo-rails
turbolinks
uglifier
webdrivers (= 5.3.0)
Expand Down
10 changes: 7 additions & 3 deletions lib/react_on_rails/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ def self.configuration
i18n_output_format: nil,
components_subdirectory: nil,
make_generated_server_bundle_the_entrypoint: false,
defer_generated_component_packs: true
defer_generated_component_packs: true,
# forces the loading of React components
force_load: false
)
end

Expand All @@ -53,7 +55,8 @@ class Configuration
:server_render_method, :random_dom_id, :auto_load_bundle,
:same_bundle_for_client_and_server, :rendering_props_extension,
:make_generated_server_bundle_the_entrypoint,
:defer_generated_component_packs
:defer_generated_component_packs,
:force_load

# rubocop:disable Metrics/AbcSize
def initialize(node_modules_location: nil, server_bundle_js_file: nil, prerender: nil,
Expand All @@ -68,7 +71,7 @@ def initialize(node_modules_location: nil, server_bundle_js_file: nil, prerender
same_bundle_for_client_and_server: nil,
i18n_dir: nil, i18n_yml_dir: nil, i18n_output_format: nil,
random_dom_id: nil, server_render_method: nil, rendering_props_extension: nil,
components_subdirectory: nil, auto_load_bundle: nil)
components_subdirectory: nil, auto_load_bundle: nil, force_load: nil)
Copy link
Contributor

@coderabbitai coderabbitai bot May 16, 2024

Choose a reason for hiding this comment

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

Consider adding a comment explaining the purpose of force_load for better maintainability.

-                   components_subdirectory: nil, auto_load_bundle: nil, force_load: nil)
+                   components_subdirectory: nil, auto_load_bundle: nil, force_load: nil) # force_load: Forces the loading of React components

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
components_subdirectory: nil, auto_load_bundle: nil, force_load: nil)
components_subdirectory: nil, auto_load_bundle: nil, force_load: nil) # force_load: Forces the loading of React components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, but add comment on line 43

Copy link
Contributor

Choose a reason for hiding this comment

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

The line 43 in the provided content of lib/react_on_rails/configuration.rb is part of the initialize method of the Configuration class. Specifically, it is where the force_load attribute is initialized. This attribute is crucial for the new functionality introduced in this pull request, which forces the loading of React components.

Here's the specific line from the file:

self.force_load = force_load

Given the context and the purpose of the force_load attribute, a suitable comment would explain that this line is setting the default behavior for the force_load configuration, which is used to control whether React components are loaded immediately.

@theforestvn88, here's a suggested comment for line 43 in lib/react_on_rails/configuration.rb:

self.force_load = force_load  # Sets the default behavior for force loading React components

This comment clarifies that the line is responsible for setting the default state of the force_load attribute, which is central to the new functionality introduced in your pull request. If this aligns with your expectations, you can add it to the code.

self.node_modules_location = node_modules_location.present? ? node_modules_location : Rails.root
self.generated_assets_dirs = generated_assets_dirs
self.generated_assets_dir = generated_assets_dir
Expand Down Expand Up @@ -106,6 +109,7 @@ def initialize(node_modules_location: nil, server_bundle_js_file: nil, prerender
self.auto_load_bundle = auto_load_bundle
self.make_generated_server_bundle_the_entrypoint = make_generated_server_bundle_the_entrypoint
self.defer_generated_component_packs = defer_generated_component_packs
self.force_load = force_load
end
# rubocop:enable Metrics/AbcSize

Expand Down
8 changes: 8 additions & 0 deletions lib/react_on_rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,14 @@ def internal_react_component(react_component_name, options = {})
"data-trace" => (render_options.trace ? true : nil),
"data-dom-id" => render_options.dom_id)

if render_options.force_load
component_specification_tag.concat(
content_tag(:script, %(
ReactOnRails.reactOnRailsComponentLoaded('#{render_options.dom_id}');
).html_safe)
)
end

load_pack_for_generated_component(react_component_name, render_options)
# Create the HTML rendering part
result = server_rendered_react_component(render_options)
Expand Down
4 changes: 4 additions & 0 deletions lib/react_on_rails/react_component/render_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ def logging_on_server
retrieve_configuration_value_for(:logging_on_server)
end

def force_load
retrieve_configuration_value_for(:force_load)
end

def to_s
"{ react_component_name = #{react_component_name}, options = #{options}, request_digest = #{request_digest}"
end
Expand Down
4 changes: 4 additions & 0 deletions node_package/src/ReactOnRails.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@ ctx.ReactOnRails = {
ClientStartup.reactOnRailsPageLoaded();
},

reactOnRailsComponentLoaded(domId: string): void {
ClientStartup.reactOnRailsComponentLoaded(domId);
},

/**
* Returns CSRF authenticity token inserted by Rails csrf_meta_tags
* @returns String or null
Expand Down
19 changes: 19 additions & 0 deletions node_package/src/clientStartup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,25 @@ export function reactOnRailsPageLoaded(): void {
forEachReactOnRailsComponentRender(context, railsContext);
}

export function reactOnRailsComponentLoaded(domId: string): void {
debugTurbolinks(`reactOnRailsComponentLoaded ${domId}`);

const railsContext = parseRailsContext();

// If no react on rails components
if (!railsContext) return;

const context = findContext();
if (supportsRootApi) {
context.roots = [];
}

const el = document.querySelector(`[data-dom-id=${domId}]`);
if (!el) return;

render(el, context, railsContext);
}

function unmount(el: Element): void {
const domNodeId = domNodeIdForEl(el);
const domNode = document.getElementById(domNodeId);
Expand Down
1 change: 1 addition & 0 deletions node_package/src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ export interface ReactOnRails {
setOptions(newOptions: {traceTurbolinks: boolean}): void;
reactHydrateOrRender(domNode: Element, reactElement: ReactElement, hydrate: boolean): RenderReturnType;
reactOnRailsPageLoaded(): void;
reactOnRailsComponentLoaded(domId: string): void;
authenticityToken(): string | null;
authenticityHeaders(otherHeaders: { [id: string]: string }): AuthenticityHeaders;
option(key: string): string | number | boolean | undefined;
Expand Down
5 changes: 5 additions & 0 deletions spec/dummy/Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,10 @@ GEM
timeout (0.4.1)
tins (1.32.1)
sync
turbo-rails (2.0.6)
actionpack (>= 6.0.0)
activejob (>= 6.0.0)
railties (>= 6.0.0)
turbolinks (5.2.1)
turbolinks-source (~> 5.2)
turbolinks-source (5.2.0)
Expand Down Expand Up @@ -423,6 +427,7 @@ DEPENDENCIES
spring (~> 4.0)
sprockets (~> 4.0)
sqlite3 (~> 1.6)
turbo-rails
turbolinks
uglifier
webdrivers (= 5.3.0)
Expand Down
6 changes: 6 additions & 0 deletions spec/dummy/app/controllers/pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ def data
}.merge(xss_payload)
}

@app_props_hello_from_turbo_stream = {
helloTurboStreamData: {
name: "Mrs. Client Side Rendering From Turbo Stream"
}.merge(xss_payload)
}

@app_props_hello_again = {
helloWorldData: {
name: "Mrs. Client Side Hello Again"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<%= turbo_frame_tag 'hello-turbo-stream' do %>
<%= button_to "send me hello-turbo-stream component", turbo_stream_send_hello_world_path %>
<% end %>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<%= turbo_stream.update 'hello-turbo-stream' do %>
<%= react_component("HelloTurboStream", props: @app_props_hello_from_turbo_stream, force_load: true) %>
<% end %>
7 changes: 7 additions & 0 deletions spec/dummy/client/app/packs/client-bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,20 @@ import 'core-js/stable';
import 'regenerator-runtime/runtime';
import 'jquery';
import 'jquery-ujs';
import '@hotwired/turbo-rails';

import ReactOnRails from 'react-on-rails';

import HelloTurboStream from '../startup/HelloTurboStream';
import SharedReduxStore from '../stores/SharedReduxStore';

ReactOnRails.setOptions({
traceTurbolinks: true,
turbo: true,
});

ReactOnRails.register({
HelloTurboStream
});

ReactOnRails.registerStore({
Expand Down
30 changes: 30 additions & 0 deletions spec/dummy/client/app/startup/HelloTurboStream.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import PropTypes from 'prop-types';
import React, { useState, useRef } from 'react';
import RailsContext from '../components/RailsContext';
justin808 marked this conversation as resolved.
Show resolved Hide resolved

import css from '../components/HelloWorld.module.scss';

const HelloTurboStream = ({ helloTurboStreamData, railsContext }) => {
const [name, setName] = useState(helloTurboStreamData.name);
const nameDomRef = useRef(null);

const handleChange = () => {
setName(nameDomRef.current.value);
};

return (
<div>
<h3 className={css.brightColor}>Hello, {name}!</h3>
{railsContext && <RailsContext {...{ railsContext }} />}
</div>
);
};

HelloTurboStream.propTypes = {
helloTurboStreamData: PropTypes.shape({
name: PropTypes.string,
}).isRequired,
railsContext: PropTypes.object,
};

export default HelloTurboStream;
2 changes: 2 additions & 0 deletions spec/dummy/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,6 @@
get "image_example" => "pages#image_example"
get "context_function_return_jsx" => "pages#context_function_return_jsx"
get "pure_component_wrapped_in_function" => "pages#pure_component_wrapped_in_function"
get "turbo_frame_tag_hello_world" => "pages#turbo_frame_tag_hello_world"
post "turbo_stream_send_hello_world" => "pages#turbo_stream_send_hello_world"
end
1 change: 1 addition & 0 deletions spec/dummy/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"@babel/preset-env": "7",
"@babel/preset-react": "^7.10.4",
"@babel/runtime": "7.17.9",
"@hotwired/turbo-rails": "^8.0.4",
"@rescript/react": "^0.10.3",
"babel-loader": "8.2.4",
"babel-plugin-macros": "^3.1.0",
Expand Down
20 changes: 20 additions & 0 deletions spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,26 @@ class PlainReactOnRailsHelper
it { is_expected.not_to include '<span id="App-react-component-0"></span>' }
it { is_expected.to include '<div id="App-react-component-0"></div>' }
end

describe "'force_load' tag option" do
let(:force_load_script) do
%(
ReactOnRails.reactOnRailsComponentLoaded('App-react-component-0');
).html_safe
end

context "with 'force_load' == true" do
subject { react_component("App", force_load: true) }

it { is_expected.to include force_load_script }
end

context "without 'force_load' tag option" do
subject { react_component("App") }

it { is_expected.not_to include force_load_script }
end
end
end

describe "#redux_store" do
Expand Down
10 changes: 10 additions & 0 deletions spec/dummy/spec/system/integration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,16 @@ def finished_all_ajax_requests?
end
end

describe "TurboStream send react component", :js do
subject { page }

it "force load hello-world component immediately" do
visit "/turbo_frame_tag_hello_world"
click_on "send me hello-turbo-stream component"
expect(page).to have_text "Hello, Mrs. Client Side Rendering From Turbo Stream!"
end
end

describe "Pages/client_side_log_throw", :ignore_js_errors, :js do
subject { page }

Expand Down
18 changes: 18 additions & 0 deletions spec/dummy/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2056,6 +2056,19 @@
resolved "https://registry.yarnpkg.com/@discoveryjs/json-ext/-/json-ext-0.5.6.tgz#d5e0706cf8c6acd8c6032f8d54070af261bbbb2f"
integrity sha512-ws57AidsDvREKrZKYffXddNkyaF14iHNHm8VQnZH6t99E8gczjNN0GpvcGny0imC80yQ0tHz1xVUKk/KFQSUyA==

"@hotwired/turbo-rails@^8.0.4":
version "8.0.5"
resolved "https://registry.yarnpkg.com/@hotwired/turbo-rails/-/turbo-rails-8.0.5.tgz#18c2f0e4f7f952307650308590edf5eb9544b0d3"
integrity sha512-1A9G9u28IRAl0C57z8Ka3AhNPyJdwfOrbjr+ABZk2ZEUw2QO7cJ0pgs77asUj2E/tzn1PgrxrSVu24W+1Q5uBA==
dependencies:
"@hotwired/turbo" "^8.0.5"
"@rails/actioncable" "^7.0"

"@hotwired/turbo@^8.0.5":
version "8.0.5"
resolved "https://registry.yarnpkg.com/@hotwired/turbo/-/turbo-8.0.5.tgz#abae6dad018a891e4286e87fa0959217e3866d5a"
integrity sha512-TdZDA7fxVQ2ZycygvpnzjGPmFq4sO/E2QVg+2em/sJ3YTSsIWVEis8HmWlumz+c9DjWcUkcCuB+muF08TInpAQ==

"@jridgewell/gen-mapping@^0.1.0":
version "0.1.1"
resolved "https://registry.yarnpkg.com/@jridgewell/gen-mapping/-/gen-mapping-0.1.1.tgz#e5d2e450306a9491e3bd77e323e38d7aff315996"
Expand Down Expand Up @@ -2116,6 +2129,11 @@
schema-utils "^3.0.0"
source-map "^0.7.3"

"@rails/actioncable@^7.0":
version "7.2.0"
resolved "https://registry.yarnpkg.com/@rails/actioncable/-/actioncable-7.2.0.tgz#dee66d21bc125a9819dc8080ce896eac78d8c63f"
integrity sha512-crcsPF3skrqJkFZLxesZoyUEt8ol25XtTuOAUMdLa5qQKWTZpL8eLVW71bDCwKDQLbV2z5sBZ/XGEC0i+ZZa+A==

"@rescript/react@^0.10.3":
version "0.10.3"
resolved "https://registry.yarnpkg.com/@rescript/react/-/react-0.10.3.tgz#a2a8bed6b017940ec26c2154764b350f50348889"
Expand Down
1 change: 1 addition & 0 deletions spec/react_on_rails/react_component/render_options_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
replay_console
raise_on_prerender_error
random_dom_id
force_load
].freeze

def the_attrs(react_component_name: "App", options: {})
Expand Down
Loading