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

Move StimulusReflex::Channel to app/ and allow for a configurable parent channel #346

Merged
merged 2 commits into from
Nov 1, 2020

Conversation

leastbad
Copy link
Contributor

@leastbad leastbad commented Nov 1, 2020

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

Feature/enhancement and bug fix

Description

This PR is 100% a carbon-copy of @dark-panda's #322, adapted to use the new initializer structure introduced in #339. @dark-panda did all of the hard work on this and I just needed to move things along in service of a timely v3.4.0 release.

Feature/Enhancement

Allows the parent class of StimulusReflex::Channel to be configurable via StimulusReflex.configuration.parent_channel. This can be done via the initializer:

StimulusReflex.configure do |config|
  config.parent_channel = 'SomeChannelThatsNotStandard'
end

It defaults to ApplicationCable::Channel.

Bug Fix

This came about while attempting an upgrade because for some reason @dark-panda's app's ApplicationCable::Channel implementation was not being loaded correctly, but only in certain situations. For instance, passenger in would not load the file, but puma would. There are mysterious forces at work in the Rails loading mechanisms and all of the various pieces that touch it, and they sometimes have different behaviour.

In putting together this PR, a second issue was discovered that is not specific to my app. Because SR is overriding ApplicationCable::Channel#initialize and then doing a require on the user-supplied implementation, the user-supplied implementation gets skipped on the first call to a channel's #initialize method. Thus, if the user supplies their own ApplicationCable::Channel#initialize method, it will only start to be used after at least one ActionCable connection is made, as that will cause the SR implementation to be called first, which itself then overwrites its own implementation via the require. Because this PR avoids overriding ApplicationCable::Channel in any way, we avoid this oddity. As to why anyone would override ApplicationCable::Channel#initialize is another matter, but regardless, this PR avoids this behaviour.

Why should this be added

It is possible that someone would want to provide a different parent class to StimulusReflex::Channel, or otherwise name their base channel something else entirely. This allows them to configure this behaviour, with the default being as it was before.

It avoids a small issue discovered when calling a user-supplied ApplicationCable::Channel#initialize.

Reduces code complexity by avoiding having to override ApplicationCable::Channel in SR.

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 the enhancement New feature or request label Nov 1, 2020
@leastbad leastbad added this to the 3.4.0 milestone Nov 1, 2020
@leastbad leastbad merged commit 4dfda40 into stimulusreflex:master Nov 1, 2020
@leastbad leastbad deleted the parent_channel branch November 1, 2020 16:55
leastbad added a commit that referenced this pull request Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant