Fire callbacks when targets are added or removed #367

seanpdoyle · 2021-01-20T17:10:12Z

Fire callbacks when targets are added or removed

Implements the TargetObserver to monitor when elements declaring
[data-${identifier}-target] are added or removed from a Context
after being connected and before being disconnected.

In support of iterating through target tokens, export the
TokenListObserver module's parseTokenString function.

docs/reference/targets.md

leastbad · 2021-01-23T19:41:22Z

I'm extremely happy to see this, @seanpdoyle! You rock.

seanpdoyle · 2021-03-26T20:02:08Z

I've pushed up some changes to the test suite so that it finishes its run.

It's currently failing, but that's an improvement over the previous version which ran indefinitely due to some infinite recursion.

packages/@stimulus/core/src/target_observer.ts

packages/@stimulus/core/src/target_properties.ts

seanpdoyle · 2021-03-27T01:39:17Z

I'm not sure why, but CI is failing on Windows and Safari 9.1.2 (Mac OS 10.11.6). It passes for me locally in all browsers (including Safari Version 14.0.3 (16610.4.3.1.7)) on macOS.

Is it possible that part of the suite, or any other part of this implementation requires polyfills?

sstephenson

Implementation-wise, this is looking better since the move to TokenListObserver. I'm not sure I understand the motivation for firing callbacks only on targets that appear after the controller connects, though. That really seems to limit the possible use cases.

My intuition is that target lifecycle callbacks would work similarly to value changed callbacks. So just like we call value changed callbacks for all values present when the controller connects, I'd expect we'd also call the connected callbacks for all targets present when the controller connects. Same goes for disconnected callbacks.

The specific timeline I'd expect is:

Controller#initialize()
Controller#{name}TargetConnected()
Controller#connect()
...
Controller#{name}TargetDisconnected()
Controller#disconnect()

I've spiked this out in a separate branch, if you'd like to use it as a reference or a rebase target: https://github.com/hotwired/stimulus/compare/target-lifecycle-callbacks

docs/reference/targets.md

packages/@stimulus/core/src/target_properties.ts

packages/@stimulus/core/src/target_observer.ts

docs/reference/targets.md

packages/@stimulus/core/src/target_observer.ts

Closes [hotwired#336][] --- Implements the `TargetObserver` to monitor when elements declaring `[data-${identifier}-target]` are added or removed from a `Context` _after_ being connected and _before_ being disconnected. In support of iterating through target tokens, export the `TokenListObserver` module's `parseTokenString` function. [hotwired#336]: hotwired#336

replace Array function with a for loop

Introduce the concept of a `TargetObserverDelegate`, then make `Context` implement the interface.

seanpdoyle · 2021-04-08T13:48:49Z

docs/reference/targets.md

+  }
+
+  inputTargetDisconnected(element) {
+    element.classList.add("removed-animation")


@sstephenson I now realize that the element will already be out of the DOM, so this contrived example doesn't really illustrate the point.

One use case I had in mind was sorting, would that be a better example? Can you think of a use case where having access to the Element instance that was removed would be helpful?

seanpdoyle · 2021-04-08T14:09:42Z

@sstephenson I've incorporated your feedback and pushed up the changes. The suite passes for me locally on evergreen browsers, but is still failing in CI.

Are there polyfills we need to enable in our SauceLabs configuration?

adrienpoly · 2021-04-08T15:06:16Z

@seanpdoyle for the CI I think the issue comes from isConnected that is not supported by all browsers. https://caniuse.com/?search=isConnected

This was my first result when searching for a Polyfill
https://gist.github.com/eligrey/f109a6d0bf4efe3461201c3d7b745e8f

you might want to try to add it here to see if it makes the CI happy
https://github.com/hotwired/stimulus/blob/main/packages/%40stimulus/polyfills/index.js

seanpdoyle · 2021-04-08T15:12:11Z

Thanks for the tip @adrienpoly. The only other spot we're relying on isConnected is

stimulus/packages/@stimulus/mutation-observers/src/element_observer.ts

Line 132 in 21f2ca2

if (element.isConnected != this.element.isConnected) {

.

Since the suite isn't failing prior to this change, I wonder if that line is properly covered by our suite. Maybe those tests include a polyfill of sorts?

The implementation was sourced from the [Mozilla Developer Network documentation][poyfill] for [Node.isConnected][] [poyfill]: https://developer.mozilla.org/en-US/docs/Web/API/Node/isConnected#polyfill [Node.isConnected]: https://developer.mozilla.org/en-US/docs/Web/API/Node/isConnected

adrienpoly · 2021-04-08T16:16:26Z

It seems to remove most of the prior issues. Not sure to understand the remaining CI error and yes I think this was a missing polyfill in previous versions.

``` IE 11.0 (Windows 8.1) ERROR 133 Expected identifier, string or number 134 at @stimulus/core/dist/tests/index.js:2281:1 135 136 SyntaxError: Expected identifier, string or number 137 at ./packages/@stimulus/polyfills/index.js (@stimulus/core/dist/tests/index.js:2281:1) ``` ``` IE 11.0 (Windows 8.1) target disconnected callback when element is removed FAILED 125 Promise rejected during "target disconnected callback when element is removed": Object doesn't support property or method 'remove' 126 TypeError: Object doesn't support property or method 'remove' 127 at Anonymous function (eval code:191:25) ```

seanpdoyle · 2021-04-08T16:50:48Z

@adrienpoly thanks for the tips! The suite is now passing.

javan · 2021-04-12T01:43:19Z

The only other spot we're relying on isConnected is … Since the suite isn't failing prior to this change, I wonder if that line is properly covered by our suite.

See #161 (comment) for context.

seanpdoyle · 2021-04-12T01:47:43Z

Thanks for the context @javan. Since this polyfill is only used to pass our suite on CI in a legacy Windows environment, what do you think is the best path forward?

javan · 2021-04-12T01:53:52Z

Totally fine to add a polyfill for isConnected.

packages/@stimulus/polyfills/index.js

Add attribute-level support for monitoring changes to attributes on the marked element _and_ its descendants. The `data-mutation` syntax draws direct inspiration from the `[data-action]` syntax for routing browser events. Similar to how the Action syntax supports [event listener options][], the Mutation syntax would support [MutationObserverInit options][] like `!subtree`. The proposed hooks only cover _attribute_ mutations, since the proposal made by [hotwired#367][] should cover `childList` type mutations like the addition or removal of controller targets. One alternative could involve combining `[data-mutation]` and `[data-action]` into a single DOMTokenList, and using additional symbols like `@...` or wrapping `[...]` as a differentiators (e.g. `@aria-expanded->disclosure#toggle` or `[aria-expanded]->disclosure#toggle`). Another could push this responsibility application-side by introducing more publicly available `MutationObserver` utilities like those used for `DOMTokenList` parsing or deconstructing the `[data-action]` directives. Once available, those utilities could be used by consumers to listen for their own mutations and "route" them to actions by combining action directive parsing and `Application.getControllerForElementAndIdentifier(element, identifier)` to invoke fuctions on a `Controller` instance. [hotwired#367]: hotwired#367 [event listen options]: https://stimulus.hotwire.dev/reference/actions#options [MutationObserverInit options]: https://developer.mozilla.org/en-US/docs/Web/API/MutationObserverInit#properties

Closes hotwired#471 --- This commit re-arranges the documentation for Controller Lifecycle Callbacks to reflect the order of execution and elaborates with more details when possible. The possibilities for the callback execution order was discussed and the [current order was agreed upon][order]. [order]: hotwired#367

Closes #471 --- This commit re-arranges the documentation for Controller Lifecycle Callbacks to reflect the order of execution and elaborates with more details when possible. The possibilities for the callback execution order was discussed and the [current order was agreed upon][order]. [order]: #367

seanpdoyle force-pushed the target-callback branch 3 times, most recently from 2f8a20d to c89e2cc Compare January 20, 2021 17:32

seanpdoyle commented Jan 20, 2021

View reviewed changes

docs/reference/targets.md Outdated Show resolved Hide resolved

seanpdoyle force-pushed the target-callback branch from c89e2cc to 5513349 Compare January 20, 2021 22:09

marcoroth mentioned this pull request Jan 24, 2021

Introduce useTargetMutation stimulus-use/stimulus-use#99

Merged

seanpdoyle force-pushed the target-callback branch from 5513349 to 06338f7 Compare February 7, 2021 20:29

seanpdoyle force-pushed the target-callback branch 3 times, most recently from 723702b to 902f112 Compare March 26, 2021 20:01

seanpdoyle force-pushed the target-callback branch 2 times, most recently from 339dee3 to 4d7f686 Compare March 27, 2021 01:28

seanpdoyle commented Mar 27, 2021

View reviewed changes

packages/@stimulus/core/src/target_observer.ts Outdated Show resolved Hide resolved

seanpdoyle commented Mar 27, 2021

View reviewed changes

packages/@stimulus/core/src/target_observer.ts Outdated Show resolved Hide resolved

seanpdoyle commented Mar 27, 2021

View reviewed changes

packages/@stimulus/core/src/target_properties.ts Outdated Show resolved Hide resolved

sstephenson suggested changes Mar 30, 2021

View reviewed changes

seanpdoyle added 4 commits April 7, 2021 21:08

attempt to fix CI

78c094b

replace Array function with a for loop

Rename Added to Connected and Removed to Disconnected

86720fc

ADd documentation to the Lifecycle Callbacks page

c182b82

seanpdoyle force-pushed the target-callback branch from fc566b2 to c182b82 Compare April 8, 2021 01:30

Incorporate code review feedback

770b2fd

Introduce the concept of a `TargetObserverDelegate`, then make `Context` implement the interface.

seanpdoyle commented Apr 8, 2021

View reviewed changes

Touch up documentation

21f2ca2

seanpdoyle requested a review from sstephenson April 8, 2021 14:08

seanpdoyle force-pushed the target-callback branch from ec98572 to 1b6e95e Compare April 8, 2021 16:39

Be consistent in documentation

ef175d5

seanpdoyle commented Apr 12, 2021

View reviewed changes

packages/@stimulus/polyfills/index.js Show resolved Hide resolved

seanpdoyle mentioned this pull request Apr 27, 2021

Proposal: Mutation Observation Syntax #397

Closed

seanpdoyle closed this Apr 30, 2021

Sub-Xaero mentioned this pull request Jun 14, 2021

Fire callbacks when targets are added or removed #409

Merged

seanpdoyle deleted the target-callback branch September 15, 2021 13:27

seanpdoyle mentioned this pull request Oct 11, 2021

Docs: Fix Lifecycle Callback order #473

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fire callbacks when targets are added or removed #367

Fire callbacks when targets are added or removed #367

seanpdoyle commented Jan 20, 2021 •

edited

Loading

leastbad commented Jan 23, 2021

seanpdoyle commented Mar 26, 2021

seanpdoyle commented Mar 27, 2021

sstephenson left a comment

seanpdoyle Apr 8, 2021

seanpdoyle commented Apr 8, 2021

adrienpoly commented Apr 8, 2021

seanpdoyle commented Apr 8, 2021

adrienpoly commented Apr 8, 2021

seanpdoyle commented Apr 8, 2021

javan commented Apr 12, 2021

seanpdoyle commented Apr 12, 2021

javan commented Apr 12, 2021

Fire callbacks when targets are added or removed #367

Fire callbacks when targets are added or removed #367

Conversation

seanpdoyle commented Jan 20, 2021 • edited Loading

leastbad commented Jan 23, 2021

seanpdoyle commented Mar 26, 2021

seanpdoyle commented Mar 27, 2021

sstephenson left a comment

Choose a reason for hiding this comment

seanpdoyle Apr 8, 2021

Choose a reason for hiding this comment

seanpdoyle commented Apr 8, 2021

adrienpoly commented Apr 8, 2021

seanpdoyle commented Apr 8, 2021

adrienpoly commented Apr 8, 2021

seanpdoyle commented Apr 8, 2021

javan commented Apr 12, 2021

seanpdoyle commented Apr 12, 2021

javan commented Apr 12, 2021

seanpdoyle commented Jan 20, 2021 •

edited

Loading