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

remove strict dependencies on Active Record, Action Cable and Active Storage #222

Merged
merged 13 commits into from
Jan 3, 2023

Conversation

leastbad
Copy link
Contributor

Type of PR (feature, enhancement, bug fix, etc.)

Enhancement

Description

In an effort to be helpful, I've manually reimplemented the changes in 828c7d0 22399b3 and 24c2f0b. That is, @marcoroth wrote 100% of the code in this commit. I just wanted to help get the changes in, quickly. I'm confident that there's a fancy git cherry pick command that would do this, but I also needed to make sure it merged cleanly and since there's not that much to this PR, I just wanted to get it done. If there's a better way, close this PR without delay!

Concerns before merging:

  1. Are there new/additional things which need to be covered? I admit that I'm not 100% sure I understand why these changes were all necessary, which means I can't guarantee that any new requirements are accounted for... if any.
  2. Do we need to do similar optimizations in SR?

Fixes #207

Why should this be added

Tidying up loose ends!

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

@leastbad leastbad added proposal ruby Pull requests that update Ruby code labels Oct 21, 2022
@leastbad leastbad added this to the 5.0 milestone Oct 21, 2022
@leastbad
Copy link
Contributor Author

leastbad commented Oct 21, 2022

standardrb failed in a strange way that I do not understand: https://github.com/stimulusreflex/cable_ready/actions/runs/3294765236/jobs/5432607555

It works just fine on my local machine. Can't find ruby >= 2.7.0 is super weird.

@marcoroth
Copy link
Member

standardrb failed in a strange way that I do not understand

I guess it might be because of this line: https://github.com/stimulusreflex/cable_ready/pull/222/files#diff-2f4da7b98a9c86d38f368f6119614e27dbb9c1e4dc3d70240cb466cc211435acR24

and/or that the standard action is still running on Ruby 2.6

@leastbad
Copy link
Contributor Author

I don't know how to change how actions are run.

https://github.com/stimulusreflex/cable_ready/blob/master/.github/workflows/standardrb.yml doesn't seem to specify a Ruby version. Is there a setting somewhere for this? Nothing about runtime environment on https://github.com/stimulusreflex/cable_ready/settings/actions

@marcoroth marcoroth mentioned this pull request Oct 21, 2022
3 tasks
@marcoroth
Copy link
Member

marcoroth commented Oct 21, 2022

https://github.com/stimulusreflex/cable_ready/blob/master/.github/workflows/standardrb.yml doesn't seem to specify a Ruby version. Is there a setting somewhere for this? Nothing about runtime environment on https://github.com/stimulusreflex/cable_ready/settings/actions

I was being set by the third party GitHub Action we were using. I resolved this via #223.

But I think the tests failures we are seeing now are legit because of what seems to be missing ActiveRecord requires

@@ -1,5 +1,7 @@
# frozen_string_literal: true

require "action_view"
Copy link
Member

Choose a reason for hiding this comment

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

I guess we need to require active_record here and probably also define action_view and active_record as dependencies of the gem.

@leastbad
Copy link
Contributor Author

How do we move this one forward, @marcoroth? Are you able to give this a few minutes? It would be great to cross this issue off the list.

@leastbad
Copy link
Contributor Author

Alright, after a substantial amount of pain, I appear to have sorted out the issues preventing the tests and CI steps from passing.

Thanks to @javierg and @KonnorRogers for the assists!

I'd like to renew the push to either get this fixed and merged, or close the chapter. To reiterate, the goal of this PR is to make it possible to address #207 by making it possible to use CR in a non-Active Record project.

If this goal is possible, then we can document which steps need to be taken (in terms of requires etc) and which features are off-limits. I have never used anything but Active Record, and I barely understand zeitwerk, so I am woefully ill-prepared to take this PR over the finish line.

If this goal is not possible, we will have to break the news and this PR should be closed with regrets. @marcoroth, can you take over responsibility for this PR and the decisions motivating it? I've gone about as far as I can with it.

@leastbad
Copy link
Contributor Author

leastbad commented Jan 2, 2023

@javierg I don't want to jinx it, but I think that I have CableReady working properly without ActiveRecord in scope. It shows up in Gemfile.lock a few times, as a dependency of frameworks like ActiveStorage. However, I believe that I have *everything working without AR being required.

WindowsTerminal_U0Yl6Q0V0S

  1. rails new foo -j esbuild -O
  2. add "cable_ready", github: "stimulusreflex/cable_ready", branch: "new_installer" to Gemfile
  3. rails cable_ready:install and accept all defaults (hit enter ~5 times)
  4. change Gemfile so that the cable_ready branch is dependency_cleanup; run bundle if neccesary
  5. bin/dev in one Terminal tab
  6. open localhost:3000/example in browser
  7. rails c in a second Terminal tab
  8. execute these lines
    include CableReady::Broadcaster
    cable_ready[:example_page].text_content("#stream_from_output", text: "Hello from the console!").broadcast
  9. You should see the message you sent in your browser, and you can execute defined?(ActiveRecord) and see it's not required.

@marcoroth I am still feeling pretty fuzzy about all of this; it's not 100% clear to me what all of the shuffling that wasn't AR-specific was intended to do, or if you even finished this task before getting distracted. Some code review is appreciated, though FWIW the tests are passing.

Don't sweat the conflicts; I'll handle all of the touch-ups when it comes time to merge.

Anyhow, @javierg I am so sorry that this took so long, and I hope that it still helps. Can you please give it a few tire kicks and let us know if anything seems off?

* The big caveat for this PR is that while the Identifiable module works just fine without Active Record, the Updatable functionality is disabled. If you try to include CableReady::Updatable in a non-AR class, it will 100% blow up. That's not something we can address at this time, sorry!

@javierg
Copy link

javierg commented Jan 2, 2023

@leastbad thank you for this work, I tested your branch on a project were AR is not present and it works as expected in the basic escenarios we have.

@leastbad leastbad changed the title redo on @marcoroth's dependency-cleanup branch remove strict dependencies on Active Record, Action Cable and Active Storage Jan 3, 2023
@leastbad leastbad merged commit 34c9436 into master Jan 3, 2023
@leastbad leastbad deleted the dependency_cleanup branch January 3, 2023 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ActiveRecord dependency breaks rspec on non AR Rails implementations
3 participants