Skip to content

replace nulls in props from Rails with undefined #1273

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

Conversation

MaySoMusician
Copy link
Contributor

@MaySoMusician MaySoMusician commented May 3, 2023

Summary

closes: discussion #1272

I'm currently working a Rails+React project and eliminating nulls from the type definitions in the frontend as possible, since my team members and I have been bothered about differentiation between null and undefined in terms of typing.

This PR adds an opt-in option to make react_ujs replace all nulls in the props from Rails with undefineds, alongside the option system to allow to selectively enable each option. I've made the option default to false so that all existing projects won't break.

Read the discussion for the more detailed background.

Other Information

As soon as this gets merged, I'll also update @types/react_ujs to add type definitions of setOptions(option)

Pull Request checklist

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

@justin808
Copy link
Collaborator

First, there are no tests for this. However, we don't want to change the code.

See my comment in #1272

Let's convert this to a doc PR. I suggest we add a /docs directory.

@justin808 justin808 changed the title Add option to make UJS replace nulls in props from Rails with undefined Add DOCs to show how to replace nulls in props from Rails with undefined May 6, 2023
@MaySoMusician
Copy link
Contributor Author

@justin808
Replied you in #1272 (reply in thread). Unfortunately I won't still discard my original PR.

@justin808
Copy link
Collaborator

@MaySoMusician We now have an app for integration tests. Can you please add tests to https://github.com/reactjs/react-rails/tree/master/test/dummy for your feature and I will merge. Be sure to provide a changelog and readme update.

Thank you for your contribution!

@justin808
Copy link
Collaborator

@MaySoMusician can you please add tests.

@justin808 justin808 changed the title Add DOCs to show how to replace nulls in props from Rails with undefined replace nulls in props from Rails with undefined Jun 6, 2023
@MaySoMusician
Copy link
Contributor Author

@justin808 Sure! I will do that.

@justin808
Copy link
Collaborator

@ahangarha Let's get this merged once there are tests.

@MaySoMusician Thanks!

@MaySoMusician
Copy link
Contributor Author

@justin808 I'll be dealing with this today. Thanks for waiting for me!

@MaySoMusician MaySoMusician force-pushed the feature/add-option-to-replace-null branch 3 times, most recently from 45dc85e to b631207 Compare June 16, 2023 06:18
@MaySoMusician MaySoMusician force-pushed the feature/add-option-to-replace-null branch from b631207 to f2ae2e2 Compare June 16, 2023 06:19
Copy link
Collaborator

@ahangarha ahangarha left a comment

Choose a reason for hiding this comment

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

I recommend naming react_ujs/src/options.js as optionsHelpers.js because it contains only helper functions. The current name options.js suggests having a file with some class or object related to various options.

@ahangarha
Copy link
Collaborator

@MaySoMusician Any update?
Please rebase your branch so that your PR can trigger the automated tests.

@justin808
Copy link
Collaborator

@MaySoMusician Do you want to finish this one?

@ahangarha
Copy link
Collaborator

@justin808 @MaySoMusician I will take the lead so we can go for RC.

@ahangarha ahangarha self-assigned this Jul 25, 2023
@MaySoMusician
Copy link
Contributor Author

Sorry for my long inactivity. I have encountered so large changes in my employment relationship in recent weeks that I was unable to tackle this. I can have a time to start this tonight, that will take some time since I have to build a vm to install Python2 required by the test environments, which is not installed in recent laptops. @ahangarha @justin808

@ahangarha
Copy link
Collaborator

@MaySoMusician Happy to hear from your side.
And I hope that the change in your career ends in your favor.

I think we can wait for a day or two. Looking forward to the update.

@justin808
Copy link
Collaborator

We're about to release v3...

@ahangarha
Copy link
Collaborator

@MaySoMusician I rebased your changes on the master branch.

@ahangarha ahangarha closed this Jul 27, 2023
@ahangarha
Copy link
Collaborator

@MaySoMusician Back to this PR?

@MaySoMusician
Copy link
Contributor Author

@ahangarha Probably, yes.
And I will try to set up test environment again. #1297 might help me.

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.

3 participants