Use the full power of Stimulus

[guillaumebriday]

First thing first, I'm so happy to see this project! It's a huge deal for Stimulus and Symfony!! 👍

Under this title, I mean multiple things.

1. Using disconnect callback

I think all controllers should use the disconnect callback to destroy instances. Like the Chart one.

If I dynamically remove the node from the DOM, the chart instance will remain loaded.

connect() {
  this.chart = new Chart(this.element.getContext('2d'), payload);
}

disconnect () {
  this.chart.destroy()
  this.chart = undefined
}

2. Using data maps

Stimulus has a pretty handy API to deal with data: https://stimulusjs.org/reference/data-maps

It's cool because it's name-spaced by default, so it "forces" developer to use it in a more generic way. And it has methods to access datas.

I see lots of this.element.getAttribute('key') but they are not using data-maps

3. Improve inheritence

All controllers dispatch a custom event "extends" controllers in a some way.

I think it would be more consistent to allow developers to extends these controllers like here with instance variables and so on instead of using custom event. Because sometime, it's useful to override the behavior.

Thanks a lot for this project!

[tgalopin]

Hi Guillaume! Thanks for your feedback and thanks a lot for your ideas!

Answering your message:

1/ Using disconnect callback

You are absolutely right: I added this in the READMEs of the packages but it's a very good idea to add it on the Chart component. If you wish to create a PR for https://github.com/symfony/ux/blob/main/src/Chartjs/Resources/assets/src/controller.js, that would be awesome

2/ Using data maps

My reasoning behind not using the data API but native JS getAttribute calls is the following:

• my main problem is that it does impose a scoped name, which would be very verbose in the case of Symfony UX controllers (data-symfony-ux-chartjs-chart-view) as they come with already scoped package names
• my second thought is that the Data API is likely to change a lot in Stimulus 2: it wouldn't make much sense to base Symfony UX on it right now if it will change a lot in the coming weeks

In opposition to these two drawbacks, I don't see a huge value into using the data APIs in package controllers as they don't store a complex state for the moment. So the balance of drawbacks vs advantages went to not using the data API for now, but I can change my opinion if I missed some arguments in favour of it!

3/ Inheritance

My current reasoning regarding inheritance is based on the experience of Symfony as a framework, in which providing features using vertical inheritance has major drawbacks:

• If an application controller extends a Controller that requires an external service of something complex, one cannot create an automated test without actually calling the extended Controller, as the reference to the Controller is embedded in the app controller. Using events allow automated tests to easily simulate a behavior, in order to test the application controller more easily.
• When you provide and incentivize people to develop their application code extending package controllers, it automatically exposes your full controller yo the child controllers, meaning all the methods of the controller can be called. This means that developing a Backward Compatible controller becomes much harder, as you can't remove or update methods from your base controller without breaking the application using it.
• Finally, using events is more composable IMO: let's imagine that I develop a debug package for Symfony UX as a whole. I can listen to all events relative to the current element quite easily and chose whether to react to them of not. It's much harder to develop such a package with inheritance, as I would need to implement a controller per base controller.

Generally speaking, I feel the inheritance debate is a bit like the Inheritance vs Composition debate, and my mind favours Composition in this situation. I may be missing something of course, feel free to point it to me if so!

[tgalopin]

Oh and also! I think it would be awesome if your stimulus-components library would be compatible with Symfony UX, would you mind if I open a PR to see how it could work :) ?

[nicolas-grekas]

I think this has been discussed and addressed. Thanks for chimming in!