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

Improve Rails 6 support #800

Merged
merged 1 commit into from Mar 10, 2019

Conversation

Projects
None yet
2 participants
@sudara
Copy link
Contributor

sudara commented Mar 10, 2019

This PR removes 2 instances of the invalid (and unused) option :on for before_save callbacks in models.

Without this change, the following error is raised in Rails 6:
Unknown key: :on. Valid keys are: :if, :unless, :prepend

Please note that the :on option was a no-op before Rails 6, so this PR does not change any behavior.

However, Rails 6 introduced stricter option checking for model callbacks: rails/rails#30919

These were the only changes required to get Thredded happy on Rails 6 for http://github.com/sudara/alonetone — Impressive!🥇

@glebm

This comment has been minimized.

Copy link
Collaborator

glebm commented Mar 10, 2019

These callbacks are only supposed to be invoked once, when creating the record. Can you please change it to do that (I think it's before_create)? Thanks!

@glebm glebm merged commit 7781837 into thredded:master Mar 10, 2019

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@glebm

This comment has been minimized.

Copy link
Collaborator

glebm commented Mar 10, 2019

Nevermind, it's fine for these to run on update as well as they're idempotent. Thanks!

@sudara

This comment has been minimized.

Copy link
Contributor Author

sudara commented Mar 15, 2019

Just a heads up that I'm also currently looking into CollectionToStringsWithCacheRenderer which looks like it has trouble on Rails 6 due to this commit: rails/rails@1bc0a59#diff-c10847e19aefe462d2aaeb02ea389700

alonetone | Don't Panic! The White Theme FAQ  2019-03-15 20-57-30

Not sure yet if I will succeed, it gets complex!

@sudara

This comment has been minimized.

Copy link
Contributor Author

sudara commented Mar 15, 2019

PartialRenderer#render now returns an object (instead of a string). I've temporarily got our app happy on Rails 6 with this, but obviously it's not sustainable or backwards compatible, so we'll have to revisit: f6aa8ce

@glebm

This comment has been minimized.

Copy link
Collaborator

glebm commented Mar 16, 2019

Thank you for your investigation. Can we just do this conditionally based on Rails::VERSION::MAJOR >= 6?

@sudara

This comment has been minimized.

Copy link
Contributor Author

sudara commented Mar 18, 2019

Yeah, totally possible to switch on version, but I felt reluctant to implement without better understanding the class. Seems like it would need a tad bit of refactoring to support a switch like that. I'm also curious if this class could be the cause for some memory bloat we've been seeing...

@glebm

This comment has been minimized.

Copy link
Collaborator

glebm commented Mar 19, 2019

@sudara This class uses a thread pool to render posts in a topic.
Multi-threading here improves performance despite the GIL because rendering sometimes involves a network call (e.g. for the oneboxes).

I'm also curious if this class could be the cause for some memory bloat we've been seeing...

It does trade off a bit of memory for speed. You can find out how much by disabling multi-threading like this in the initializer:

Thredded::CollectionToStringsWithCacheRenderer.render_threads = 1

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes include:
1. Fix to the multi-threaded collection renderer.
2. Removing dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Using git versions of `roadie-rails` and `rspec-rails`.

Fixes #801
Refs #800

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800

@glebm glebm referenced this pull request Mar 31, 2019

Merged

Rails 6.0 beta support #802

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800

@glebm glebm added this to the v0.16.10 milestone Mar 31, 2019

glebm added a commit that referenced this pull request Mar 31, 2019

Rails 6.0 beta support
Changes:

1. Fixes to the multi-threaded collection renderer.
2. Removes dev dependency on `factory_bot_rails`, instead using plain
   `factory_bot` (as we don't need reloading support anyway).
3. Uses git versions of `roadie-rails` and `rspec-rails` for Rails 6
   beta for now.

Fixes #801
Refs #800
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.