Skip to content

Allow passing in controller#62

Merged
julianrubisch merged 14 commits into
stimulusreflex:masterfrom
rickychilcott:feature/allow-passing-in-controller
Oct 12, 2020
Merged

Allow passing in controller#62
julianrubisch merged 14 commits into
stimulusreflex:masterfrom
rickychilcott:feature/allow-passing-in-controller

Conversation

@rickychilcott

Copy link
Copy Markdown
Contributor

Enhancement

Description

Optionally allow passing in the controller to use to render the partials.

Fixes #54

Why should this be added

This allows specifying the Controller to be used to render partials which will allow controller specific methods and behaviors to be available in the partial rendering.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

Comment thread lib/futurism/channel.rb Outdated
Comment thread lib/futurism/channel.rb
Comment thread lib/futurism/channel.rb
@rickychilcott

Copy link
Copy Markdown
Contributor Author

I was thinking about that @scottbarrow. If @julianrubisch is onboard with this overall approach, then it'd be a simple change to make to lib/futurism/channel.rb read something like:

    def controller(signed_controller:)
      return default_controller unless signed_controller.present?

      message_verifier
        .verify(signed_controller)
        .yield_self { |controller_string| Kernel.const_get(controller_string) }
    end

    def default_controller
      Futurism.default_controller || ::ApplicationController
    end

@julianrubisch

Copy link
Copy Markdown
Contributor

Congrats folks, somehow @rickychilcott read my mind while I was pondering this while running errands. Uncanny!

I‘m all in for an overridable default controller!

@rickychilcott rickychilcott force-pushed the feature/allow-passing-in-controller branch from 2d33d9e to f6cb4bc Compare October 9, 2020 17:49

@julianrubisch julianrubisch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this all looks great, only had 2 minor questions.

I'll put this through its paces on Monday and test it on a real project 👍

Comment thread README.md Outdated
Comment thread lib/futurism/channel.rb Outdated
@rickychilcott rickychilcott force-pushed the feature/allow-passing-in-controller branch from 5cb55a1 to 8cb6134 Compare October 9, 2020 19:40
@rickychilcott

Copy link
Copy Markdown
Contributor Author

I think this all looks great, only had 2 minor questions.

I'll put this through its paces on Monday and test it on a real project 👍

Please do. I made some more changes after this review. If you don't mind either building or writing up some quick directions on Monday on how to test this gem in a real app, that'd be helpful. I know how to point my Gemfile to a local gem, but not clear on how to do that with yarn.

It'd be good to have a dummy app in this repo or elsewhere to more easily test.

Thanks and have a great weekend!

@julianrubisch

Copy link
Copy Markdown
Contributor

Regarding testing your local version against a real app: use yarn link

As for a test app, the stimulus_reflex_harness has a futurism branch (thanks @leastbad): https://github.com/leastbad/stimulus_reflex_harness/tree/futurism

@rickychilcott

Copy link
Copy Markdown
Contributor Author

Thanks for the hints about testing locally, @julianrubisch. I was able to test this PR using my FuturismAuthSpike project with both setting Futurism.default_controller= and by passing it in via the futurize method. Both work with the latest commits.

Thanks for building out the test harness @leastbad. Unfortunately, the branch is referencing old syntax, so it was a bit easier to use my own project.

@julianrubisch would you like to have a purpose-built test harness for futurism so that it makes future testing easier? My hope would be to exercise many of the functions and be able to make PR review faster. Ideally, it would also have minimal dependencies (i.e. using sqlite3 and the async adapter locally) to facilitate even faster testing and setup.

If that's the case, I can re-work my auth project to be a bit more generic or bring @leastbad's into line.

@leastbad

Copy link
Copy Markdown
Contributor

I'd love to see a Futurism-specific test app with minimal dependencies. I have a feeling that you'd do an awesome job.

@julianrubisch

Copy link
Copy Markdown
Contributor

would you like to have a purpose-built test harness for futurism so that it makes future testing easier? My hope would be to exercise many of the functions and be able to make PR review faster.

@rickychilcott that would certainly be awesome! Maybe I can also set heroku review apps up for this...

@julianrubisch

Copy link
Copy Markdown
Contributor

Cherry picked 5820988 into master

Comment thread lib/futurism/channel.rb Outdated
@julianrubisch julianrubisch merged commit a41a1bd into stimulusreflex:master Oct 12, 2020
@julianrubisch

Copy link
Copy Markdown
Contributor

👏

@rickychilcott rickychilcott deleted the feature/allow-passing-in-controller branch October 12, 2020 10:41
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.

ApplicationController context

3 participants