stimulusreflex / stimulus_reflex

Build reactive applications with the Rails tooling you already know and love.
https://docs.stimulusreflex.com
MIT License
2.28k stars 172 forks source link

controller inheritance does not seem to work #283

Closed scottbarrow closed 4 years ago

scottbarrow commented 4 years ago

Bug Report

Describe the bug

Inherited js controllers do not seem to be getting called by reflex. supporting documentation https://docs.stimulusreflex.com/patterns#application-controller-pattern

To Reproduce

Setup new rails app add reflex with bin/rails g stimulus_reflex SomeReflex, add element with data-reflex attribute and a parent element with a data-controller attribute

with

import { Application } from "stimulus";
import { definitionsFromContext } from "stimulus/webpack-helpers";

import StimulusReflex from "stimulus_reflex";
import consumer from "./channels/consumer";

const application = Application.start();
const context = require.context("controllers", true, /_controller\.js$/);
application.load(definitionsFromContext(context));
StimulusReflex.initialize(application, { consumer });
#app/javascript/controllers/application_controller.js
import { Controller } from "stimulus";
import StimulusReflex from "stimulus_reflex";
  connect() {
    console.log("connect ap");
    StimulusReflex.register(this);
  }
  initialize() {
    console.log("initialize ap");
  }
  disconnect() {
    console.log("disconnect ap");
  }
  beforeReflex(element, reflex) {
    console.log("beforeReflex ap");
  }
  reflexSuccess(element, reflex, error) {
    console.log("reflexSuccess ap");
  }
  reflexError(element, reflex, error) {
    console.log("reflexError ap");
  }
  afterReflex(element, reflex) {
    console.log("afterReflex ap");
  }
#app/javascript/controllers/child_controller.js
import ApplicationController from "./application_controller";

export default class extends ApplicationController {
  // Stimulus callbacks
  connect() {
    console.log("connect child");
    StimulusReflex.register(this);
  }
  initialize() {
    console.log("initialize child");
  }
  disconnect() {
    console.log("disconnect child");
  }
  // SR callbacks
  beforeReflex(element, reflex) {
    console.log("beforeReflex child");
    // document.body.classList.add('wait')
  }
  reflexSuccess(element, reflex, error) {
    console.log("reflexSuccess child");
    // show success message etc...
  }
  reflexError(element, reflex, error) {
    console.log("reflexError child");
    // show error message etc...
  }
  afterReflex(element, reflex) {
    console.log("afterReflex child");
    // document.body.classList.remove('wait')
  }
}

Expected behavior

Reflex to trigger and call lifecycle methods initialize child connect child beforeReflex child disconnect child reflexSuccess child afterReflex child connect child

Actual behaviour

initialize ap connect ap beforeReflex ap disconnect ap reflexSuccess ap afterReflex ap connect ap

If I don't use controller inheritance the controller somewhat works as expected, only the before* and beforeReflex methods are called

leastbad commented 4 years ago

Hi Scott. I'm not sure what's going on in here.

Your index.js looks fine.

Your application_controller.js is not a valid JS class. It might work better if you do this:

#app/javascript/controllers/application_controller.js
import { Controller } from "stimulus";
import StimulusReflex from "stimulus_reflex";
export default class extends Controller {
  connect() {
    console.log("connect ap");
    StimulusReflex.register(this);
  }
  initialize() {
    console.log("initialize ap");
  }
  disconnect() {
    console.log("disconnect ap");
  }
  beforeReflex(element, reflex) {
    console.log("beforeReflex ap");
  }
  reflexSuccess(element, reflex, error) {
    console.log("reflexSuccess ap");
  }
  reflexError(element, reflex, error) {
    console.log("reflexError ap");
  }
  afterReflex(element, reflex) {
    console.log("afterReflex ap");
  }

Because your ApplicationController is FUBAR, it's not surprising that your ChildController is behaving strangely. Also, you're not importing StimulusReflex into your ChildController, so it really can't work.

Now, if you fix up your ApplicationController, you still need to call super or else you're just clobbering the class that you're extending.

#app/javascript/controllers/child_controller.js
import ApplicationController from "./application_controller";

export default class extends ApplicationController {
  // Stimulus callbacks
  connect() {
    console.log("connect child");
    super.connect()
  }
  initialize() {
    console.log("initialize child");
    super.initialize()
  }
  disconnect() {
    console.log("disconnect child");
    super.disconnect()
  }
  // SR callbacks
  beforeReflex(element, reflex) {
    console.log("beforeReflex child");
    super.beforeReflex(arguments)
    // document.body.classList.add('wait')
  }
  reflexSuccess(element, reflex, error) {
    console.log("reflexSuccess child");
    super.reflexSuccess(arguments)
    // show success message etc...
  }
  reflexError(element, reflex, error) {
    console.log("reflexError child");
    super.reflexError(arguments)
    // show error message etc...
  }
  afterReflex(element, reflex) {
    console.log("afterReflex child");
    super.afterReflex(arguments)
    // document.body.classList.remove('wait')
  }
}

As I have previously stated, I personally don't like the inherited Stimulus controller pattern. That's just my opinion. I'd much rather have some less-DRY classes that are easier to follow.

Here's how I'd do it:

import { Controller } from "stimulus";
import StimulusReflex from "stimulus_reflex";

export default class extends Controller {
  connect() {
    console.log("connect");
    StimulusReflex.register(this);
  }
  initialize() {
    console.log("initialize");
  }
  disconnect() {
    console.log("disconnect");
  }
  beforeReflex(element, reflex) {
    console.log("beforeReflex", arguments);
  }
  reflexSuccess(element, reflex, error) {
    console.log("reflexSuccess", arguments);
  }
  reflexError(element, reflex, error) {
    console.log("reflexError", arguments);
  }
  afterReflex(element, reflex) {
    console.log("afterReflex", arguments);
  }
}

Nice, clean and no bullshit. Does that work any better for you?

scottbarrow commented 4 years ago

Thanks @leastbad apologies, there was a copy paste error, my application_controller does in fact look like your suggestion. What I was missing was the super call in the child. Inheritance does in fact work