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

Add element and target attribute change callbacks #460

Closed

Conversation

seanpdoyle
Copy link
Contributor

Caveat

I still believe that declaring mutation callbacks in the same style as event Action Routing is more aligned with Stimulus idioms and has the same self-documenting aspirations. With that being said, I think investigating more generic Controller callbacks could serve as an interesting exercise.

Changes

Closes #445


Implement attributeChange callbacks for both a controller's element
and its targets.

The attributeChanged and [name]TargetAttributeChanged callback
signatures are styled after the Custom Element's
attributeChangedCallback. The target variations accept the target
element as the first argument, but are otherwise the same.

It feels more appropriate to strive for parity in the interface than it
is to yield the underlying MutationRecord to call sites.

@@ -28,6 +28,8 @@ Method | Invoked by Stimulus…
initialize() | Once, when the controller is first instantiated
connect() | Anytime the controller is connected to the DOM
[name]TargetConnected(target: Element) | Anytime a target is connected to the DOM
attributeChanged(attributeName: string, oldValue: string | null, newValue: string | null) | Anytime the controller element's attributes change
[name]TargetAttributeChanged(target: Element, attributeName: string, oldValue: string | null, newValue: string | null) | Anytime a target element's attributes change
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs corresponding prose elsewhere.

@@ -28,6 +28,8 @@ Method | Invoked by Stimulus…
initialize() | Once, when the controller is first instantiated
connect() | Anytime the controller is connected to the DOM
[name]TargetConnected(target: Element) | Anytime a target is connected to the DOM
attributeChanged(attributeName: string, oldValue: string | null, newValue: string | null) | Anytime the controller element's attributes change
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs corresponding prose elsewhere. It's also worth noting that Values are better suited for observing data- prefixed changes, and that attributeChanged is better for global attributes like [disabled], [hidden], or aria-* prefixed attributes.

@rik
Copy link
Contributor

rik commented Nov 16, 2021

One thing that this API as it is does not allow: get a callback when character data changes.

@seanpdoyle
Copy link
Contributor Author

@rik could you outline a use case where character data observations could be valuable?

Closes hotwired#445

---

Implement [attributeChange][] callbacks for both a controller's element
and its targets.

The `attributeChanged` and `[name]TargetAttributeChanged` callback
signatures are styled after the Custom Element's
`attributeChangedCallback`. The target variations accept the target
element as the first argument, but are otherwise the same.

It feels more appropriate to strive for parity in the interface than it
is to yield the underlying [MutationRecord][] to call sites.

[attributeChange]: https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements#using_the_lifecycle_callbacks
[MutationRecord]: https://developer.mozilla.org/en-US/docs/Web/API/MutationRecord
@rik
Copy link
Contributor

rik commented Nov 18, 2021

The use case was monitoring the label of an <option> to copy it in another element.

(It is not the most compelling use case because it does not really fit the HTML-over-the-wire spirit)

@seanpdoyle
Copy link
Contributor Author

@rik what is triggering the <option> text content change? Is the element itself being modified by client-side code or a server-sent response?

One alternative (if you own the code responsible) is to mark the <option> as a target (with [data-$IDENTIFIER-target]), then call Element.replaceWith instead of modifying innerHTML (or however you're modifying the content).

That way, the [IDENTIFIER]TargetConnected(target) would fire and you'd have an opportunity to respond.

If this isn't code that you control, maybe a separate callback could be justifiable. I dislike introducing a single, less specific callback that accepts a MutationRecord instance, since type-based guards could become tedious for most applications.

Maybe a [name]TargetCharacterDataChanged(textContents: string) or [name]TargetTextContentsChanged(textContents: string) could work.

@seanpdoyle
Copy link
Contributor Author

I'm closing this in favor of exploring this behavior in an external package first: https://github.com/seanpdoyle/stimulus-mutation.

@seanpdoyle seanpdoyle closed this Aug 27, 2022
@marcoroth
Copy link
Member

I still believe that this functionality should make it into upstream Stimulus in some way or another!

@seanpdoyle
Copy link
Contributor Author

I still believe that this functionality should make it into upstream Stimulus in some way or another!

I agree. Starting with an external package enables consumers to pick and choose the way they interact with the observers: either callbacks or Actions with [data-mutation] routing declarations.

Ideally, one or the other would win out, and the maintenance burden would be halved. Having said that, I'd be open to supporting both styles.

@brunoprietog
Copy link

I hope this will be merged someday

@seanpdoyle
Copy link
Contributor Author

I hope this will be merged someday

@brunoprietog @marcoroth I appreciate that both of you expressed interest in and support for this feature.

Could you both share an example of a use case you imagine would be simplified by this feature? How are you achieving that outcome now? Which style do you prefer: the callbacks or the data-action mutation descriptor routing?

@marcoroth
Copy link
Member

marcoroth commented Sep 5, 2022

There are a few use cases where you'd want to listen for changed attributes. Either when other controllers or scripts are setting attributes or when external plugins/libraries are setting attributes so that you can react to it. I also can see some uses cases for a [name]TargetTextContentsChanged(textContents: string) callback, i.e contenteditable comes to mind.

Personally I like the callbacks approach better. To me the data-mutation attribute somehow feels redundant, because it's basically doing the same as the data-action attribute, except for mutations.

We could probably get the same behavior if we let Stimulus dispatch events instead, so you could pick up the mutations as regular action descriptors in the data-action attribute. Using the recently added Action options in #567 we could also support the descriptor syntax you proposed for the data-mutation attribute, just with the benefit that we don't need to introduce a new attribute which basically does the same.

Or is there another reason why you would want to split the regular actions from mutations?

@brunoprietog
Copy link

I also like the callback better.

Currently, I'm using them to execute a function when the state of a details element changes.

It feels unnecessary to always have to declare the same actions for things that will always be repeated in the controllers.

For example, in a dialog, you will always need to listen for a keydown to close it when you press the escape key, a click@document to close it when you click outside of the dialog, and a toggle to run the method that should do extra things when it opens or closes.

A callback avoids having to put more descriptors in the html for these kinds of common and repetitive cases. This allows me to avoid listening for the toggle event.

Anyway, there is nothing to prevent adding and removing event listeners in the controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add [target]TargetChanged and elementChanged callbacks
4 participants