Support to_dom_selector for operation selectors

[jaredcwhite]

Resolves #134

FYI, I added to_s onto the final path for the updated selector logic, but that might be a breaking change (since previously a non-String object would be converted via to_json, not to_s). There's also the outstanding question of whether we need to issue a warning or exception if something mangled gets through like an inspect string with class name, etc.

Happy to work on updated docs as well, just let me know.

[leastbad]

Thank you, Jared! This is awesome. I cannot fathom why we wouldn't accept this concept for inclusion in v5. We do need to rework this so that it happens inside of dom_id, and not adjacent to it.

dom_id actually gets called both in StimulusReflex (the library) and by users of StimulusReflex creating their own Reflexes. I think it's exciting to consider that people could pass objects to morph that can generate their own DOM selectors.

The other consideration is that the original intention of dom_id was to prepend a # character to the selector, so that it would magically with the ActionView equivalent. If someone's component exposes a to_dom_selector method, someone accessing it with dom_id can take for granted that it will handle the # for them.

I'd like to propose that we add a method to identifiable.rb:

def identifiable?(obj)
  obj.is_a?(ActiveRecord::Relation) || obj.is_a?(ActiveRecord::Base) || obj.respond_to?(:to_dom_selector)
end

This would allow us to change line 35 of operation_builder.rb:

options["selector"] = identifiable?(previous_selector) ? dom_id(previous_selector) : previous_selector

And that would allow us to add the to_dom_selector check to dom_id directly, enabling it across the whole framework:

    def dom_id(record, prefix = nil)
      prefix = prefix.to_s.strip if prefix

      id = if record.respond_to?(:to_dom_selector)
        record.to_dom_selector
      elsif record.is_a?(ActiveRecord::Relation)
        [prefix, record.model_name.plural].compact.join("_")
      elsif record.is_a?(ActiveRecord::Base)
        ActionView::RecordIdentifier.dom_id(record, prefix)
      else
        [prefix, record.to_s.strip].compact.join("_")
      end

      "##{id}".squeeze("#").strip
    end

This also solves the to_s concern with the fall-through case, I think!

What do you think?

---

I should be fine with the docs (dom_id is already written) but of course I'll pass the section by you for suggestions and improvements.

[jaredcwhite]

Makes sense to consolidate inside of dom_id…however I'd advocate for just:

return record.to_dom_selector

in the case of that being available. I don't think assuming it must be an actual id is warranted. Could be any selector. rando-tag[activated] or body > header > nav. That's just my preference anyway.

[leastbad]

stares at Jared's comment for 10-12 minutes before replying

I feel like I get where you're coming from, and I thought about ways we could pull this off. For example, what if to_dom_selector methods could return a string or a {selector: "a > b[any-old-thing"}? If it returns a string, it would get a # prepended, and if it came back as a hash, we could pass back the raw selector value from the object. It's a bit of a hack, but it's one I could swallow.

However, the function is dom_id, and the folks calling it expect a fully formed #id to emerge. They are, after all, already able to pass any selector string they like.

That means that instead of making to_dom_selector a special case, instead the developer could just pass my_obj.to_dom_selector as a selector value. I don't explicitly call to_partial_path on my model instances often, but the fact that I can is a cool trick that I have deployed >0 times in my career.

The nice thing about this approach is that while it's slightly less terse and magical, it still checks your box for the object being responsible for where it is sent to.

I'm open to exploring either of these paths, or a third!

[jaredcwhite]

Would love additional input on this…I guess my dilemma is I've personally used the original Rails' dom_id in an app approximately (checks notes) 0 times (well, prior to Hotwire anyway), so I have no muscle memory on how that works or what is expected. And, to be frank, when it dawned on me Turbo Frame IDs (for example) are literally just standard id attributes (as opposed to frame-id or something), that was a big eww for me. I consider using id in the DOM somewhat of an anti-pattern because there's no guarantee of uniqueness whatsoever. Dev A could write something with id="widget" and somebody else could come along and write something with id="widget" and the two have absolutely no commonality whatsoever yet they end up on the same page together and then who knows what will break. Rails' dom_id sort of makes sense as you probably get uniqueness because of the combined AR class name + record id. But if we run with this generic to_dom_selector idea and somebody just returns foo and who knows how many #foo elements are actually on a page, that seems counterproductive.

Anyway, if we do keep using IDs only, I'd recommend changing the method name to to_dom_id. Having string or hash return values with different behavior seems confusing to me. 🤷‍♂️

[leastbad]

We're going to have to agree to disagree on a number of points, because we're not looking to move away from using id at this time. CR is a sharp knife and we have four years of experience gathering problem reports; happy to say that non-unique IDs are not something folks are complaining about. We think that id is a standard, expected and powerful tool. CR also has a very different API mouthfeel than anything Turbo Frames is doing.

Similarly, we're not looking to make a break from dom_id. It's short, it's in wide use in production SR projects, and it isn't constrained to folks who are thinking component-first. A small but relevant point is that we use it and like it, or it wouldn't be there. 😉

Good news, though: I have a third path to propose and I think that you'll like it. (I just realized that you probably meant changing the name of to_dom_selector to to_dom_id... not changing dom_id to to_dom_id. 🤦 So forgive me if I'm regurgitating the obvious.)

Why not support both? It's fair to say that we want this to work for as many use cases as possible:

    def dom_id(record, prefix = nil)
      prefix = prefix.to_s.strip if prefix

      id = if record.respond_to?(:to_dom_selector)
        return record.to_dom_selector
      elsif record.respond_to?(:to_dom_id)
        record.to_dom_id
      elsif record.is_a?(ActiveRecord::Relation)
        [prefix, record.model_name.plural].compact.join("_")
      elsif record.is_a?(ActiveRecord::Base)
        ActionView::RecordIdentifier.dom_id(record, prefix)
      else
        [prefix, record.to_s.strip].compact.join("_")
      end

      "##{id}".squeeze("#").strip
    end

I can sell that! What do you think?

[jaredcwhite]

Seems perfectly reasonable to me! I'll redo the PR and see what other feedback we can collect.

[leastbad]

Thanks, Jared... this looks great! I am trying to figure out why the test suite is giving us a void value expression error, though. That's a new one for me.

Do you see anything weird? Did you run standardrb?

[jaredcwhite]

Do you see anything weird? Did you run standardrb?

Strange… All seems a-OK on my side

[jaredcwhite]

@leastbad Wow, so apparently earlier versions of Ruby wouldn't allow you to return directly within an if statement. That got fixed apparently in 2.7. There's possibly an indication of a backport to 2.6 but as we see here on GH that isn't guaranteed.

https://bugs.ruby-lang.org/issues/11143

Anyway we might want to redo that dom_id logic a bit.

[leastbad]

Nice catch! Also: I hate this kind of thing. Also: I can't believe that I've never hit this case before.

Even though I wish we could bump to >=2.7, I don't think that this is a strong enough justification.

In your opinion, what's the least fugly way to set a flag around line 9 and then exit with the value around line 19 if the flag is true?

[jaredcwhite]

I switched it out to a guard statement up top. Let me know what you think :)

[leastbad]

Duh, of course!

I think it's great. Let me make sure the others agree.

[hopsoft]

I like what this accomplishes; though, I am a little wary of mixing dom_selector semantics within the dom_id implementation. Having said that, advanced users should know what they're doing when they implement to_dom_selector for use with CableReady.

[marcoroth]

Sorry for being late to the party!

This looks awesome and I love where this is heading!

I have one thought to add: if an object defines both to_dom_selector and to_dom_id shouldn't we then use to_dom_id? From what I can tell we are currently using to_dom_selector in that case.

[leastbad]

You're right that in the current implementation, dom selector is plucked first. The fuzzy reasoning behind this was to handle the odd case first, and then treat every other branch in the if as an id.

What are the pros and cons of swapping the priority from selector to id?