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

Idea: ability to pass any object which responds to to_dom_selector as a selector #134

Closed
jaredcwhite opened this issue Jun 4, 2021 · 6 comments · Fixed by #135
Closed

Comments

@jaredcwhite
Copy link
Contributor

Trying out the cable_car functionality completely standalone (aka literally a blank folder with gem "cable_ready", "~> 5.0.0.pre1" in the Gemfile—pretty cool that it works!), and right away I noticed that if I pass any random object in as a selector (custom object, Set, whatever), basically it ends up as whatever the to_json representation is in the dispatch. (I know dom_id is a thing but that only works for ActiveRecord.) Needless to say, the to_json output of most (all?) objects isn't actually anything suitable for a selector.

What would be really nifty is if there was some kind of check to see if the object responds to something CableReady is optional looking for, say, to_dom_selector. That way you could write this:

class FancyPants
  def to_dom_selector
    "#fancy-pants"
  end
end

cable_ready.inner_html(FancyPants.new, html: "<p>Very fancy!</p>")

Basically my goal is I never want to write selectors directly in CableCar operation statements, I'd always prefer those come from encapsulated components of one sort or another. (This is the same complaint I have with Turbo…I never want to write Turbo Frame IDs at the point I'm rendering Turbo Streams).

If there's interest in something like this, I could work on a PR, but I wanted to see what you think first. Thanks!

@leastbad
Copy link
Contributor

leastbad commented Jun 4, 2021

  1. I love love love this idea. Please do.
  2. Thank you for tipping me off to the notion of people passing in non-AR/string values to selector. We will discuss!

@KonnorRogers
Copy link
Contributor

Oh wow! This so good! Reminds me exactly like "to_partial_path" for rendering! Genius 🧠

@jaredcwhite
Copy link
Contributor Author

Cool, that way it could work with generic ActiveModels or literally anything else. Will do.

@leastbad
Copy link
Contributor

leastbad commented Jun 4, 2021

I've been starring at add_operation_method, specifically line 35. I'm not going to change anything (because I know you're working on it) but I do want to make sure that we address both of your ideas... including the fact that passing something that doesn't easily convert to a String isn't a meaningful selector.

Once you've implemented your idea to check objects for a to_dom_selector method, what - if anything - should we do about selector values that don't easily convert to Strings?

Part of me wants to raise an exception if the first two characters (after a to_s) are "#<" or something... but then I am reminded that CableReady doesn't enforce any validations on any of its options, even for one that is as important as selector.

So perhaps the correct approach is to do nothing and just let people generate invalid selectors? It won't break CableReady, it just won't successfully find any matching elements in the DOM.

Anyone have opinions? I'm leaning towards doing nothing but I'm listening if you think differently.

@jaredcwhite
Copy link
Contributor Author

@leastbad My approach will probably be to extract line 35 out into a standalone method, so that piece of logic can be refined over time. I don't know that we need to solve all potential gotchas immediately. Maybe a warning, rather than an exception, but I don't know if you have an existing "warning" system per se.

@leastbad
Copy link
Contributor

leastbad commented Jun 6, 2021

I am pretty sure that the right thing is to do nothing, at this point. The issue is that while some CR options could be verified with a simple type check or allowlist, others have contextual ranges. Still others are modified by other options. Attempting to validate it seems like a fool's errand given that everyone using the library so far seems to have no need to be told that arbitrary objects converted to json aren't valid/useful parameters.

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

Successfully merging a pull request may close this issue.

3 participants