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

docs: get started docs r-r version #1325

Closed
wants to merge 2 commits into from
Closed

docs: get started docs r-r version #1325

wants to merge 2 commits into from

Conversation

adnjoo
Copy link

@adnjoo adnjoo commented Dec 24, 2023

Summary

edit get-started.md to include latest version of react-rails #1324

Other Information

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

- [] Add/update test to cover these changes

  • Update documentation
    - [ ] Update CHANGELOG file

@@ -51,7 +51,7 @@ Also update the Babel configuration in the `package.json` file:

4. Install `react-rails`:
```bash
$ bundle add 'react-rails' --strict
$ bundle add 'react-rails' --strict --version '3.1.1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This way, we should update the docs for every single release. Isn't it?

Copy link
Author

Choose a reason for hiding this comment

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

hmm how about like bundle add 'react-rails' --strict --version '>=3.1.1'

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is important to install "exact" version for both the Ruby gem and node package. This is emphasized in the documentation as well.

@adnjoo adnjoo requested a review from ahangarha December 26, 2023 04:58
@@ -51,7 +51,7 @@ Also update the Babel configuration in the `package.json` file:

4. Install `react-rails`:
```bash
$ bundle add 'react-rails' --strict
$ bundle add 'react-rails' --strict --version '>=3.1.1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use "exact" version.

Copy link
Collaborator

@ahangarha ahangarha Dec 26, 2023

Choose a reason for hiding this comment

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

But I see one point. --strict doesn't count the patch version and when we have fixes and bump the patch value in the version, --strict doesn't help.

If there is any better proposal (than manually fix the version) please share.

CC: @Judahmeek @justin808

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not seeing the value of any changes. What am I missing?

@G-Rath
Copy link
Contributor

G-Rath commented Dec 29, 2023

Is there a reason why we're suggesting using --strict in the first place?

@ahangarha
Copy link
Collaborator

ahangarha commented Dec 29, 2023

Is there a reason why we're suggesting using --strict in the first place?

Because bundle add react-rails adds gem "react-rails", "~> 3.1" which means any 3.x version of react-rails. Since react-rails have both Ruby and Node sides, we need to ensure we use "exact" and the same versions for both.

Any suggestion?

I can think of making some hooks in our release script to bump the version in the documentation as well. Not ideal.

@G-Rath
Copy link
Contributor

G-Rath commented Dec 29, 2023

ah right it's react_ujs and it's installed by rails generate react:install which is why it's not listed in the getting started docs.

As I've pushed for over in Shakapacker, I don't think we should be encouraging people to pin to exact versions since it tends to make things more brittle in managing updates (especially for security fixes), and the whole ideal of patch versions is they should not be breaking - I know it's not impossible for there to be a dependency between the Ruby and JS versions of the package for a particular bug fix, but in reality how often has that actually happened?

I think the better long-term path is to help the user ensure the versions are in sync like what's being done in Shakapacker - effectively shakacode/shakapacker#156, which probably could be done by just copying over the version detection logic stuff? Noting though I have actually somewhat deliberately ignored this part of react-rails for the time being to focus on the package_json work because unlike with Shakapacker I have not run into any version mismatch issues and I think it might make sense for some of the JS version stuff to actually live in package_json, but don't want to rush into it and end up with a poor design.

tbh it feels like a bug in Bundler that --strict does not give you the latest patch version, given there's no other way to dynamically resolve the latest version with --strict...

@ahangarha
Copy link
Collaborator

tbh it feels like a bug in Bundler that --strict does not give you the latest patch version, given there's no other way to dynamically resolve the latest version with --strict...

I agree. Twice, I thought about it but then left the issue. Perhaps they cannot change the behavior of the strict option but maybe can add --strict-patch option. Strict should mean STRICT!!! But... 😄

@justin808
Copy link
Collaborator

@ahangarha @G-Rath ready to merge?

@justin808
Copy link
Collaborator

If this changes, then we should also change react_on_rails for the same issue.

We have this documentation: https://www.shakacode.com/react-on-rails/docs/getting-started/

@ahangarha
Copy link
Collaborator

@justin808
I am not in favor of this change.

The only solution I can think of is to update the docs in our release script.
Otherwise, I think we should keep the docs the way it is.

@adnjoo adnjoo closed this Jan 11, 2024
@ahangarha
Copy link
Collaborator

By the way, thanks for the initiative.
@adnjoo

@adnjoo
Copy link
Author

adnjoo commented Jan 12, 2024

@ahangarha np, thanks for reviewing :)

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.

4 participants