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 an invoke_method operation #198

Merged
merged 5 commits into from
Jun 1, 2022

Conversation

jaredcwhite
Copy link
Contributor

@jaredcwhite jaredcwhite commented Apr 18, 2022

This adds an operation which allows you to call a JavaScript method for an element, whether that's a builtin HTML element or a custom element. Or you can use window as the receiver instead of an element. Example:

cable_car.invoke_method "#some-element", method: :doSomething,
                                         args: ["wow", { very: "cool" }], # you can (optionally) pass any number of args via an array
                                         delay: 1000 # as with all operations now, you can delay it by X ms

cable_car.invoke_method receiver: :window, method: "Turbo.visit", args: ["/path"]
  • Documentation
  • Tests (not sure how?)

@marcoroth
Copy link
Member

I love this! ❤️

I'm wondering if we additionally should support a way of invoking a function on the window or document object instead of "just" elements.

@leastbad
Copy link
Contributor

I'm wondering if we additionally should support a way of invoking a function on the window or document object instead of "just" elements.

100% but what's the least janky API? If there was only one, we could structure it as a boolean that falls through to the selector as default.

We could accept a string option called target that defaults to selector but could also be window or document, and then we use a switch that falls through to the selector?

@leastbad
Copy link
Contributor

... or a string option called receiver, which I suspect is more intuitive than target in this context.

@jaredcwhite
Copy link
Contributor Author

I worry that would expose limitations of the API on a regular basis…like what if you had a global object so somebody would expect to be able to call window.someGlobalThing.doSomething(...)? Would we need to support nested receivers then? Then at that point CR becomes a Ruby-ish alternative to wild-west .js.erb style code. I'm not necessarily saying that's bad, just that we should consider the sharp knives we might be selling.

@leastbad
Copy link
Contributor

That is a good point, but technically isn't this already an issue with selector-based objects as well?

A Web Component could expose an Object that has several methods, no?

Part of the reason I suggested receiver is because there's essentially three root objects that might hold function references. It seems like the question of which one might deserve to stand alone from the question of nested objects.


As it happens, I have a theoretical solution / syntax to propose. Note that I'm not lobbying for this approach, just stating my first instinct of how I might tackle it.

Once your receiver is established, then it seems like we could just split on the . character and construct a multi-level call:

cable_ready operation: "call_method", receiver: "window", method: "Foo.Bar.baz"

I think this is pretty reasonable, and no more exploitable than a script tag already is.

@jaredcwhite
Copy link
Contributor Author

@leastbad dayyuum, that's tempting. 😄

@hopsoft
Copy link
Contributor

hopsoft commented Apr 19, 2022

I wonder if that last example would feel more appropriate as:

cable_ready operation: "call_method", receiver: "window.Foo.Bar", method: "baz"

@leastbad
Copy link
Contributor

I am just chewing over all of this.

We have three potential options in play, because we shant forget selector.

Actually, let me back up...

In CR, most operations operate on a selector, but a minority ignore selector and are hard-wired to operate on window or document. The important takeaway here is that you cannot specify window or document as a selector. There's no logic [at present?] to parse out window and document and handle them as JS objects, instead of DOM elements.

This is what lead to me proposing a receiver; otherwise, we could have just used the selector.

My goals here are to expose functionality in a maximally useful way that is consistent with the call signatures of the other operations. I believe we also share a powerful motivation to eliminate confusion without restricting creativity. I bring this up because there's a sharp edge that I am actively trying to round off:

# bad, in my opinion
cable_ready operation: "call_method", selector: "#test", receiver: "Foo.Bar", method: "baz"
cable_ready operation: "call_method", receiver: "window.Foo.Bar", method: "baz"

# good, in my opinion
cable_ready operation: "call_method", selector: "#test", method: "Foo.Bar.baz"
cable_ready operation: "call_method", receiver: "window", method: "Foo.Bar.baz"

Do you see what I see? I am trying to avoid a "if you're doing a DOM element, no receiver is required but if you're working with window or document, you include the name of the object as a string inside of the receiver" scenario. That, to me, does not pass the sniff test.

@jaredcwhite
Copy link
Contributor Author

Maybe even better:

cable_ready operation: "invoke", method: "Foo.Bar.baz"

@hopsoft and some others suggested invoke which is growing on me

and we don't need receiver if we assume that it's always window unless a selector is specified. even document is easily accessible as a member of window

@jaredcwhite
Copy link
Contributor Author

I'm also realizing this re-raises the specter of "is it a method or a function?" …because if you declare a global function (setting ESM/imports stuff aside):

function printStr(str) {
  console.log(str)
}

it becomes a method on the window object. 😄

window.printStr("Hello world")

which you could then invoke… 🧐

@leastbad
Copy link
Contributor

@hopsoft and some others suggested invoke which is growing on me

This operation will be called invoke_method. @hopsoft has always historically been the decider in these moments, and with the exception of "nothing morph" (still makes my head spin 😉) he's proven pretty damn wise in this regards. My bias is always towards shorter names, but a consistent, guessable API (see dispatch_event) has to take priority. I'm actually happy to have the naming locked in so that we can focus on the fun part, API-wise.

we don't need receiver if we assume that it's always window unless a selector is specified

Oh, man... you had me super excited for about two minutes. The problem is that the way CR works - and it's been this way for a long time - is that an undeclared selector falls through to document. So there's no way to just assume window because we have no way to know if they specified document, or if it was just the default.

Still, I love where you were headed with this. Not having to specify a receiver would be pretty nice. Technically, right now you don't have to specify a selector OR a receiver when you're invoking methods on document. Too bad that people don't tend to put methods there. 🎭

If you have any other ideas that would get us there, keep 'em coming.

@jaredcwhite
Copy link
Contributor Author

Ah, that document fallback totally escaped me. Well…I think I'm on board with your syntax then!

@leastbad
Copy link
Contributor

Do you want to take a crack at the implementation? Happy to help but don't want to step on toes.

@jaredcwhite
Copy link
Contributor Author

Yup

@marcoroth
Copy link
Member

marcoroth commented Apr 20, 2022

Maybe I'm not following 100%, but if you have this call...

cable_ready.invoke_method(receiver: "window", method: "Foo.Bar.baz")

... you are assuming that Bar is a property on the respective Foo object.

But what if those are not actual properties, but functions? Functions which might even take arguments? Should we support something like this as well?

In JavaScript this could look like:

Foo.getBar().getBaz(0).doSomethingOnBaz("with arguments", 2)

I feel like method chaining would be the "most natural" in that case. Maybe, something like:

cable_ready
  .invoke_method(receiver: "window", method: "Foo.getBar")
  .invoke_method(chain: true, method: "getBaz", args: [0])
  .invoke_method(chain: true, method: "doSomethingOnBaz", args: ["with arguments", 2])

Is this too far?

@leastbad
Copy link
Contributor

Strong first reaction: that's a lot. I think that at some point, you need a sane cut-off before it makes more sense to write a custom operation or just call something that calls something else.

Is this partially motivated by having no idea how this would actually be implemented? Sure. But mostly it just seems complicated AF for a vanishingly small surface area of opportunity.

@jaredcwhite
Copy link
Contributor Author

Well, speaking only for myself, the number of times I expect to call methods off of global JS objects is roughly, er, never. 😄 My original use case was wanting to trigger a custom element to do something via a method call, so that doesn't even need any method chaining. I do like the idea of method chaining, but only in the simple property-based form until you get to the final method signature. I think beyond that it becomes too fiddly.

@jaredcwhite jaredcwhite changed the title Add a call_method operation Add an invoke_method operation Apr 25, 2022
@jaredcwhite
Copy link
Contributor Author

I wonder if we should also update setProperty so it supports a window receiver as well as nested foo.bar.baz functionality.

Copy link
Contributor

@leastbad leastbad left a comment

Choose a reason for hiding this comment

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

We're close! Great work on this one so far.

javascript/operations.js Outdated Show resolved Hide resolved
javascript/operations.js Outdated Show resolved Hide resolved
javascript/operations.js Outdated Show resolved Hide resolved
@jaredcwhite
Copy link
Contributor Author

Sounds good, I'll try to get to these changes soon. Been outta commission for a few days now, not sure exactly when I'll be back to full speed ahead…

@leastbad
Copy link
Contributor

This one is close, too. 😉

@jaredcwhite
Copy link
Contributor Author

@leastbad Updated to support document as a receiver, and I also realized a bunch of method calls could potentially fail due to calling the method as a function directly rather than using .apply and setting the receiver as the this arg. So this should be much more robust now.

@jaredcwhite jaredcwhite requested a review from leastbad May 27, 2022 17:37
@leastbad leastbad merged commit 44446f0 into stimulusreflex:master Jun 1, 2022
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 this pull request may close these issues.

None yet

4 participants