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

Add ClientPropsExtension support #1413

Merged

Conversation

Judahmeek
Copy link
Contributor

@Judahmeek Judahmeek commented Dec 26, 2021

The goal of this PR is to add the possibility of handling the props differently for the client and the server:

Changes:

  • Added rendering_props_extension configuration which takes a module with a adjust_props_for_client_side_hydration method, which is used to process props differently for server/client if prerender is set to true

This change is Reviewable

@gscarv13 gscarv13 marked this pull request as ready for review December 27, 2021 16:49
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 22 files at r2.
Reviewable status: 1 of 23 files reviewed, 2 unresolved discussions (waiting on @Judahmeek and @justin808)


docs/guides/configuration.md, line 135 at r2 (raw file):
Let's rename

config.rendering_props_extension

and have one method on there:

adjust_props_for client_side_hydration

and document that:

This configuration allows props to be used for server-side-rendering and stripped out of the props passed for client hydration.

(and adjust as you see fit)


spec/dummy/Gemfile.lock, line 68 at r2 (raw file):

      activesupport (= 7.0.0)
      marcel (~> 1.0)
      mini_mime (>= 1.1.0)

Use another PR for Rails 7

@justin808
Copy link
Member

@Judahmeek can you rebase on master?

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r1, 21 of 22 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Judahmeek)


docs/guides/configuration.md, line 133 at r2 (raw file):

  # To handle client-side & server-side props differently,
  # Add a module with a modify_props method that expects the component's name & props hash

Let's use:

  config.rendering_props_extension = RenderingPropsExtension

with method:

adjust_props_for client_side_hydration

docs/guides/configuration.md, line 228 at r2 (raw file):

module ClientPropsExtension
  # The modify_props method will be called by ReactOnRails::ReactComponent::RenderOptions if config.client_props_extension is defined
  def self.modify_props(component_name, props)

see above on the naming


lib/react_on_rails/configuration.rb, line 54 at r2 (raw file):

                  :i18n_dir, :i18n_yml_dir, :i18n_output_format,
                  :server_render_method, :random_dom_id,
                  :same_bundle_for_client_and_server, :client_props_extension

see above on naming


lib/react_on_rails/react_component/render_options.rb, line 32 at r2 (raw file):

      def client_props
        props_extension = ReactOnRails.configuration.client_props_extension
        props_extension ? props_extension.modify_props(react_component_name, props.clone) : props

if somebody used "" then we'd evaluate as true.

use

props_extension.present?

spec/dummy/config/initializers/react_on_rails.rb, line 18 at r2 (raw file):

end

module ClientPropsExtension

see renaming at top

Copy link
Contributor Author

@Judahmeek Judahmeek left a comment

Choose a reason for hiding this comment

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

Rebased!

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @justin808)


docs/guides/configuration.md, line 133 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Let's use:

  config.rendering_props_extension = RenderingPropsExtension

with method:

adjust_props_for client_side_hydration

Done.


docs/guides/configuration.md, line 135 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Let's rename

config.rendering_props_extension

and have one method on there:

adjust_props_for client_side_hydration

and document that:

This configuration allows props to be used for server-side-rendering and stripped out of the props passed for client hydration.

(and adjust as you see fit)

Done.


docs/guides/configuration.md, line 228 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

see above on the naming

Done.


lib/react_on_rails/configuration.rb, line 54 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

see above on naming

Done.


lib/react_on_rails/react_component/render_options.rb, line 32 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

if somebody used "" then we'd evaluate as true.

use

props_extension.present?

Done.


spec/dummy/Gemfile.lock, line 68 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Use another PR for Rails 7

Done.


spec/dummy/config/initializers/react_on_rails.rb, line 18 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

see renaming at top

Done.

@Judahmeek Judahmeek force-pushed the gscarv13/create-configuration-for-clientPropsExtension branch from b01b641 to d8c4ad4 Compare January 5, 2022 01:53
@justin808
Copy link
Member

@Judahmeek please get CI to pass.

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Reviewed 23 of 23 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Judahmeek)


lib/react_on_rails/react_component/render_options.rb, line 32 at r4 (raw file):

      def client_props
        props_extension = ReactOnRails.configuration.rendering_props_extension
        props_extension.present? && props_extension.respond_to?(:adjust_props_for_client_side_hydration) ? props_extension.adjust_props_for_client_side_hydration(react_component_name, props.clone) : props

if present but not responding, let's raise a helpful message


spec/dummy/bin/rails, line 3 at r4 (raw file):

#!/usr/bin/env ruby
APP_PATH = File.expand_path("../config/application", __dir__)
require_relative "../config/boot"

how is this related to this PR?


spec/dummy/client/app/components/HelloWorldProps.jsx, line 8 at r4 (raw file):

  const [name, setName] = useState(props.helloWorldData.name);
  // a trick to display a client-only prop value without creating a server/client conflict

awesome!


spec/dummy/Gemfile.lock, line 68 at r2 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Done.

seems many extraneous files still

@Judahmeek Judahmeek force-pushed the gscarv13/create-configuration-for-clientPropsExtension branch from d8c4ad4 to 7aaa277 Compare January 5, 2022 06:02
Copy link
Contributor Author

@Judahmeek Judahmeek left a comment

Choose a reason for hiding this comment

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

Reviewable status: 17 of 23 files reviewed, 3 unresolved discussions (waiting on @justin808)


lib/react_on_rails/react_component/render_options.rb, line 32 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

if present but not responding, let's raise a helpful message

Done.


spec/dummy/client/app/components/HelloWorldProps.jsx, line 8 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

awesome!

Done.


spec/dummy/bin/rails, line 3 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

how is this related to this PR?

Done.

@Judahmeek Judahmeek requested a review from justin808 January 5, 2022 06:05
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Judahmeek)


lib/react_on_rails/react_component/render_options.rb, line 38 at r5 (raw file):

          end

          Rails.logger.warn "ReactOnRails: your rendering_props_extension module is missing the "\

that is warn, not raise

see my comment!

it's too easy to miss something in the logs

Copy link
Contributor Author

@Judahmeek Judahmeek left a comment

Choose a reason for hiding this comment

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

Reviewable status: 22 of 23 files reviewed, 1 unresolved discussion (waiting on @justin808)


lib/react_on_rails/react_component/render_options.rb, line 38 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

that is warn, not raise

see my comment!

it's too easy to miss something in the logs

My bad. In my defence, you said "raise a helpful message", not "throw an error".

@justin808 justin808 merged commit 38156fb into master Jan 6, 2022
@justin808 justin808 deleted the gscarv13/create-configuration-for-clientPropsExtension branch January 6, 2022 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants