Replace nulls in props from Rails with undefined #1293

ahangarha · 2023-07-27T19:30:29Z

Update/Rebase to master for #1273 PR.

Judahmeek

@ahangarha we need a test for this functionality

MaySoMusician · 2023-08-03T10:57:52Z

I tried to set up the test environment with appraisal but couldn't get it. At least we have to upgrade appraisal to the latest because the currently-specified version of it doesn't correctly install the dependencies by bundle exec appraisal install

ahangarha · 2023-08-03T17:47:50Z

the currently-specified version of it doesn't correctly install the dependencies by bundle exec appraisal install

@MaySoMusician May you be more specific? Have you tried to work from this branch?

ahangarha · 2023-08-07T18:50:51Z

With changes in e77835e, the logic is implemented in Ruby side. The implementation may seem tricky and risky, but I think because of the nature of json, I could manage to match only key":null cases and ensure no other null strings would get replaced.

I would like to see if any input can break this logic. I included a highly possible similar match in the tests to ensure it doesn't get replaced.

@MaySoMusician May you please check if this solution offers the exact result you are looking for?

justin808 · 2023-08-10T08:16:05Z

lib/react/rails/component_mount.rb

+
+        # This regex matches key:value with null values while ensuing no string with similar
+        # pattern gets matched. It doesn't include null values in arrays.
+        props.to_json.gsub(/([^\\]":)null([,}\]])/, '\1undefined\2')


Regexps are great for many units tests to demonstrate the regexp is correct.

I have made some tests to ensure the regex match "key":null and only them (not the one similar to them. I think I have tested the most similar situation but I would like to get examples that break the regex.

justin808 · 2023-08-10T08:16:51Z

react_ujs/index.js

@@ -108,7 +118,8 @@ var ReactRailsUJS = {
      var className = node.getAttribute(ujs.CLASS_NAME_ATTR);
      var constructor = ujs.getConstructor(className);
      var propsJson = node.getAttribute(ujs.PROPS_ATTR);
-      var props = propsJson && JSON.parse(propsJson);
+      var props = propsJson && (ujs.options.replaceNull ? replaceNullWithUndefined(JSON.parse(propsJson))


why not a separate var for the parse result to avoid duplicating that bit?

If the ruby solution is good, all js implementations will be removed.

ahangarha · 2023-08-15T14:06:31Z

The commits for updating lock files are extracted into a PR #1297 to keep the PR focused

justin808 · 2023-08-15T22:46:21Z

Let's rebase on master.

lcmen · 2023-08-16T04:53:37Z

lib/react/rails/component_mount.rb

-            data[:react_props] = (props.is_a?(String) ? props : props.to_json)
+            data[:react_props] = props_to_json(
+              props,
+              null_to_undefined: Dummy::Application.config.react.null_to_undefined_props


@justin808 should we use Dummy::Application here? Dummy is just for tests. I suspect it should be Rails.configuration / Rails.application.config (both are the same), no?

Yes. Agree.

@ahangarha can you get a PR in for this ASAP?

Or @lcmen if you can get us a PR, I'll merge ASAP.

Extra credit if you provide a test that shows this bug.

I think the PR was not ready for merge. To me it was still a WIP with good progress.

I am on it

Hey folks,

Just wanted to chime in that this exact issue is breaking the 3.1.0 gem for me on a fresh Rails app.

Following the installation steps outlined in the README produces:

> rails s => Booting Puma => Rails 7.0.7 application starting in development => Run `bin/rails server --help` for more startup options Puma starting in single mode... * Puma version: 5.6.7 (ruby 3.1.2-p20) ("Birdie's Version") * Min threads: 5 * Max threads: 5 * Environment: development * PID: 19068 * Listening on http://127.0.0.1:3000 * Listening on http://[::1]:3000 Use Ctrl-C to stop Started GET "/" for 127.0.0.1 at 2023-08-20 20:15:25 -0400 Processing by HomeController#index as HTML Rendering layout layouts/application.html.erb Rendering home/index.html.erb within layouts/application Rendered home/index.html.erb within layouts/application (Duration: 2.8ms | Allocations: 4135) Rendered layout layouts/application.html.erb (Duration: 2.9ms | Allocations: 4247) Completed 500 Internal Server Error in 8ms (ActiveRecord: 0.0ms | Allocations: 8205) ActionView::Template::Error (uninitialized constant React::Rails::ComponentMount::Dummy null_to_undefined: Dummy::Application.config.react.null_to_undefined_props ^^^^^): 2: <p>Find me in app/views/home/index.html.erb</p> 3: 4:  5: <%= react_component("HelloWorld", { greeting: "Hello from react-rails." }) %> app/views/home/index.html.erb:5 ^C- Gracefully stopping, waiting for requests to finish === puma shutdown: 2023-08-20 20:21:16 -0400 === - Goodbye! Exiting

Please upgrade to 3.1.1.

lcmen · 2023-08-16T04:57:22Z

Hey! I believe this PR makes our controller specs to crash with a following error:

ActionView::Template::Error:
       #   uninitialized constant React::Rails::ComponentMount::Dummy

It comes from Dummy::Application reference in lib/react/rails/component_mount.rb:61 line.

ahangarha · 2023-08-16T08:45:51Z

@lcmen
Thanks for reporting the issue.

MaySoMusician · 2023-08-16T09:07:52Z

Hmm... I don't think Ruby can handle this problem due to the JSON spec. A JSON can't contain undefined value so that, in the current implementation, react_ujs will fail to mountComponents where it parse the JSON string from Rails.

My very first post in the discussion has already pointed this out.

react-rails passes the values from Rails to React via data-react-props HTML attribute with JSON, in the specification of which undefined is not defined (no pun intended). Due to this, in the part of converting them to JSON, we have no choice than converting Ruby's nils to JSON's nulls.

Or if you missed my post, you can recognize this limitation by testing the library in a real application (or the test suite, if available...)

lcmen · 2023-08-16T09:25:39Z

The more I think about this, the more I agree with @justin808 response that this should not be a responsibility of the library. What I would suggest as an alternative, is a some kind of serializer callback function that could be customized via config option. It would simply take props and run custom function to serialize props. Something like this (pseudo code):

def props_to_json(props, options = { null_to_undefined: false })
  return props if props.is_a?(String)
  
  return Rails.configuration.react_props_serializer.call(props) if Rails.configuration.react_props_serializer.present?

  # rest of the code
end

This way library gives an escape hatch for projects that require more serialization flexibility but does without imposing any additional constraints.

…eplace-null-updated" This reverts commit 00b1254, reversing changes made to 95908b8.

…eplace-null-updated" (#1300) This reverts commit 00b1254, reversing changes made to 95908b8.

ahangarha changed the title ~~Feature/add option to replace null updated~~ Replace nulls in props from Rails with undefined - updated Jul 27, 2023

ahangarha requested a review from justin808 July 27, 2023 19:31

ahangarha added this to the 3.0 milestone Jul 27, 2023

Judahmeek requested changes Jul 28, 2023

View reviewed changes

ahangarha self-assigned this Jul 28, 2023

ahangarha modified the milestones: 3.0, 3.1 Aug 1, 2023

ahangarha changed the title ~~Replace nulls in props from Rails with undefined - updated~~ WIP - Replace nulls in props from Rails with undefined - updated Aug 7, 2023

ahangarha marked this pull request as draft August 7, 2023 18:51

justin808 requested changes Aug 10, 2023

View reviewed changes

ahangarha force-pushed the feature/add-option-to-replace-null-updated branch from e77835e to 2ba7e74 Compare August 13, 2023 17:12

ahangarha and others added 8 commits August 14, 2023 17:23

Add ruby logic to replace null with undefiend in props json

8cf31a4

Update CHANGELOG.md

29b768e

Update docs

d35843f

Update ToC in README file

62bac8a

Update lock files for react-rails v3

01d0feb

Move the changelog entry out of v3

4e7a1c6

Make props_to_json a pure function with options

4ee47c3

Improve tests and add tests for arrays

3f8f482

ahangarha force-pushed the feature/add-option-to-replace-null-updated branch from 2ba7e74 to 3f8f482 Compare August 14, 2023 15:14

ahangarha added 3 commits August 14, 2023 20:03

Add array support

7a2b497

Add test for nested arrays

ed87d9a

Fix rubocop issue

edf889f

ahangarha mentioned this pull request Aug 15, 2023

Update lock files in tests for version 3 #1297

Merged

Judahmeek changed the title ~~WIP - Replace nulls in props from Rails with undefined - updated~~ Replace nulls in props from Rails with undefined Aug 15, 2023

Judahmeek marked this pull request as ready for review August 15, 2023 22:42

justin808 merged commit 00b1254 into master Aug 15, 2023

justin808 deleted the feature/add-option-to-replace-null-updated branch August 15, 2023 22:47

lcmen reviewed Aug 16, 2023

View reviewed changes

ahangarha added a commit that referenced this pull request Aug 16, 2023

Revert "Merge pull request #1293 from reactjs/feature/add-option-to-r…

8ca775d

…eplace-null-updated" This reverts commit 00b1254, reversing changes made to 95908b8.

Judahmeek pushed a commit that referenced this pull request Aug 16, 2023

Revert "Merge pull request #1293 from reactjs/feature/add-option-to-r…

2a28f50

…eplace-null-updated" (#1300) This reverts commit 00b1254, reversing changes made to 95908b8.

chtzvt mentioned this pull request Aug 21, 2023

react-rails v3.1.0 is broken #1303

Closed

Replace nulls in props from Rails with undefined #1293

Replace nulls in props from Rails with undefined #1293

Uh oh!

Conversation

ahangarha commented Jul 27, 2023

Uh oh!

Judahmeek left a comment

Choose a reason for hiding this comment

Uh oh!

MaySoMusician commented Aug 3, 2023

Uh oh!

ahangarha commented Aug 3, 2023

Uh oh!

ahangarha commented Aug 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahangarha commented Aug 15, 2023

Uh oh!

justin808 commented Aug 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcmen commented Aug 16, 2023

Uh oh!

ahangarha commented Aug 16, 2023

Uh oh!

MaySoMusician commented Aug 16, 2023

Uh oh!

lcmen commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lcmen commented Aug 16, 2023 •

edited

Loading