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

Make selector for Outlets API optional #647

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

marcoroth
Copy link
Member

@marcoroth marcoroth commented Jan 28, 2023

This pull request adds a fallback selector for outlets so you don't have necessarily provide a selector in the data-[identifier]-[outlet name]-outlet="[selector]" attribute.

The selector will default to [data-controller~=[outlet name]].

So an example like:

<div data-controller="result" class="result">...</div>
<div data-controller="result" class="result">...</div>
<div data-controller="search" data-search-result-outlet=".result">...</div>

With a controller like:

// app/javascript/controllers/search_controller.js

import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
  static outlets = ["result"]

  connect() {
    this.resultOutlets.forEach(...)
  }
}

Can now be rewritten as:

<div data-controller="result">...</div>
<div data-controller="result">...</div>
<div data-controller="search">...</div>

Of course you can still declare a selector to refine the selection of outlets.

src/core/outlet_set.ts Outdated Show resolved Hide resolved
@marcoroth marcoroth force-pushed the add-default-selector-for-outlets branch from 4daf034 to f2dcda4 Compare January 28, 2023 18:40
@marcoroth marcoroth changed the title Add fallback selector for outlets Make selector for Outlets API optional Jan 28, 2023
@marcoroth marcoroth marked this pull request as ready for review January 28, 2023 18:43
@marcoroth marcoroth force-pushed the add-default-selector-for-outlets branch from cff5e39 to c64775d Compare January 30, 2023 03:44
@seanpdoyle
Copy link
Contributor

I think I've encountered a scenario where having a default selector (the way it's proposed here) could have destructive side effects.

It's common for me to introduce an element controller to expose Element-level methods. For example, element#click would invoke Element.click, element#focus would invoke HTMLElement.focus, and even element#requestSubmit for HTMLFormElement.requestSubmit. So long as it's a one-to-one mapping between Stimulus Action Name and Method Name, I don't hesitate to add it to the element controller.

Imagine another controller that retains access to an element controller through an outlet. Let's assume it carries out some logic, then does some cleanup by invoking element#remove through the Outlet Controller interface.

If I were to typo the [data-other-controller-element-outlet] attribute name, the action would incur some nasty side effects. It would switch from executing on one (or small collection) of elements to executing on any [data-controller~=element] element on the page, of which there are many.

The side effects could range from (best case scenario) a bunch of console.error noise because the affected elements don't define method that correspond to the Stimulus Action name to (worst case scenario) element#removestripping out far-flung elements, or submitting every

` on the page (including the one's to destroy records!).

Without this change, a typo'd Outlet selector would silently fail.

@marcoroth
Copy link
Member Author

marcoroth commented Feb 2, 2023

@seanpdoyle I feel like this is kind of an "edge case" scenario. Of course you need to be careful about spelling the attribute right, but I would also argue that this could be the case for other Stimulus APIs, depending what kind of actions you perform in your controller actions. But agreed, maybe it wouldn't be as "destructive" as in your example in those cases.

I feel like defining the attribute and leaving it without a value for it to fallback to the [data-controller~=identifier] selector also feels kinda weird. Like:

<div data-[identifier]-[outlet]-outlet></div>

Because that would still require you to "write out and declare" the attribute, which is what we want to optimize with this change.

Do you have an idea how we could improve this so that something simple like typos wouldn't hurt that much?

@flickgradley
Copy link

The ability to - by default - reference all other controllers of type x on a page is super cool. It also matches how I tend to use outlets - I often invent a CSS class that I can use solely to find the other controllers, which have a name that matches the outlet.

I see the concern with the potential typo, though it does feel a bit edge-case-y to me too. Another idea is to make this opt-in - i.e. move towards options when declaring outlets:

static outlets = { result: { defaultSelector: true } }

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.

3 participants