add support for pinning scripts in remote endpoints

[titusfortner]

This is the feature that was discussed at TPAC. What wasn't mentioned was this implementation where scripts are associated with names. The idea is that intermediary nodes (like Sauce) can provide a list of named scripts available to the user without the user needing to pin them at the beginning of their tests. If scripts were only listed with UUIDs generated by the end node, this would not be possible.

I'm looking forward to hearing back on whether this PR works for how the rest of the group understood the feature request.

There's one spot in here that I'm not sure I've specified correctly, but the intent should be clear, so let me know what needs changing. Thanks!

---

Preview | Diff

[titusfortner]

I've linked my github account to my w3c account, not sure how to revalidate the failing check.

[AutomatedTester]

Do we need to worry about size of pinned scripts here or at the remote end?

[AutomatedTester]

Is it worth having a getter here? We don't have a mechanism for executing code currently the only way to know a script is there is one we have placed ourselves. What happens if a user wants to use one that a Selenium SaaS sets? Do we assume they will have access to those and need to code that from the scratch?

[titusfortner]

Not sure the value of limiting size, do browser vendors want it?

A getter might be a good idea. I'm not worried about getting a list from SaaS, that should be in documentation before the user writes the code, not something they need an open session to evaluate. A better use case would be the ability to:

pin_script(name, script) unless pinned_scripts.include?(name)

In which case maybe we would want to throw an exception for duplicated script names rather than just overwriting?

[titusfortner]

I didn't include it with this PR thinking that it might be better to implement something first and add it if people want it, but since it might affect how to handle other behavior, maybe I should.

[andreastt]

If we can’t see a use case for getting the script, let’s avoid the complexity altogether.

I do see value in a driver refusing to pin a script for whatever reason deemed necessary.

[titusfortner]

Are we discussing getting a list of names or getting the actual script? I was thinking the name list, just wanted to make sure we're discussing the same thing.

[andreastt]

Sorry, I thought you meant getting the full body of the scripts earlier.

Having an endpoint to get a list of the pinned scripts’ names does seems reasonable to me, for the same reasons I mentioned in #1445 (comment).

If there is no way of knowing which scripts are pinned in the driver, the local end must maintain these references locally. If it for whatever reason loses these references this could lead to a build-up of unused pinned scripts.

So far my thinking is we need:

• An endpoint for getting a list of the pinned scripts’ names.
• An endpoint for deleting individual pinned scripts by name.
• Optionally a shorthand method for deleting all pinned scripts.

What do you think?

[titusfortner]

TL/DR: Is it ok if the initial implementation does not have these extra commands and we add it if people request the feature? I think if we encourage SaaS & Frameworks to namespace their scripts, these shouldn't be necessary.

If we keep current PR approach of merely overwriting name collisions, then delete is definitely not necessary, and even list might not be necessary. Overwriting the script is the most forgiving thing, and I see a lot of accidental duplicated wire calls that users send to Sauce. Even though pinning a script multiple times would be inefficient, I'm not sure it makes sense for the spec to prevent something just to protect a user from being inefficient.

But even if we do change it to return an error, the use case for delete & list would only be if the user were not in complete control over their own code (e.g. the initialization code is controlled by another team and they force everyone to use a specific implementation of "something" that another team doesn't want to use). I just don't see this happening.

I'm trying to think through scenarios for this feature. And realize vast majority of people probably won't need or use this, and if they do decide to use this to tweak performance, they are more likely to figure out the better ways to do it.

1. Allow SaaS/Grid to handle it, so no pinning is necessary, delete isn't necessary, and list shouldn't be necessary because they'd be writing code based off of provided documentation or SaaS API endpoint, not trying to figure out what is available from the driver in the middle of a session. Hmm, maybe if Grid implements atoms with this, then it would make sense to get a list because our documentation... lags

driver.execute_script('sauce:visible', element)
driver.execute_script('se:visible', element)

2. Allow open source test library (or user framework code) to wrap it and manage state so it's completely encapsulated from user

class Scripts
def pin(script_name)
  js = file.read File.expand_path("../scripts/#{script_name}.js", __FILE__)
  driver.pin_script("watir:#{script_name}", js)
  pinned_scripts << script_name
end
end

class Element
def visible?
  scripts.pin(:visible) unless scripts.pinned_scripts.include?(:visible)
  execute_script('watir:visible', element)
end
end

3. Have user/framework pin ALLTHETHINGS up front, regardless of whether they'll use them. (potentially ouch)

4. Have the user manage things as needed in their code. This is the only time I can think of when you'd need to be able to get a list, and again I'm not sure why you'd want to delete things.

driver.pin_script('visible', script) unless driver.pinned_scripts.include?('visible')

If the whole point of this feature is to reduce latency and bandwidth for remote interactions, this is going to add an extra wire call every time, which isn't ideal, but maybe would provide the flexibility for teams that can't figure out how to do it in a better way? Then again, users are only going to do this themselves if they figure out it improves performance, otherwise they'll stop.

[andreastt]

We are trying to make a wholesome API that doesn’t just cater to SaaS users of WebDriver. I think it would be an unfortunate piece of design if WebDriver didn’t allow the pinned scripts to be queried.

An intermediary node (SaaS) or a local end (client) might know which scripts it has already pinned and take whatever action it feels compelled to make to prevent/allow overwriting scripts, but these are fundamentally not concerns of the specification. The specification goes to great length to not specify the concerns of the local end, meaning they are free to implement the API as they wish. But it is of course useful and indeed necessary to take into consideration the use cases local ends have.

The problem I have with a setter-only approach is the assumption that local ends are always stateful, or that they in all cases have the correct picture of the remote end’s state. Let me quickly give two examples:

1. A stateless WebDriver client might not have the ability to store states across invocations if all it knows about on the local system is the WebDriver session ID. This means unless there’s an API that allows querying the list of pinned scripts, it is impossible for it know if it’s modifying/overwriting an existing script.

2. Even with a stateful client that locally maintains references to the pinned scripts, nothing prevents an intermediary node or another client from making additional requests without the original client’s knowledge. In other words, accumulating and trusting state built up on the local end is not atomic in the sense that other endpoints might’ve manipulated the pinned scripts.

Because of the particular nature of the script pinning API and the way it accumulates data in memory, I think it is important to have primitives that allow the pinned scripts to be queried and cleared. My proposal would be to have:

• POST /session/{session id}/execute/pin with a JSON body {name: "…", script: "…"} that would overwrite any script by the same name. (Optionally you might consider returning {overwritten: true/false}, but I don’t particularly see a use case for it and it could always be introduced later.)
• GET /session/{session id}/execute/pin returning a JSON Array of scripts by their names, i.e. ["One", "Two", …].
• DELETE /session/{session id}/execute/pin/{?name} that deletes a single script by name if name is given, or all pinned scripts.

This mirrors accurately how you might design other RESTful APIs for your web app or whatever.

Of course we can contain this PR to the first API and follow up on the others later, but I feel it is important to have a clear picture in mind of where we are heading.

[jgraham]

Is there a corresponding wpt PR adding tests for the new feature?

[titusfortner]

@jgraham web-platform-tests/wpt#19675

[andreastt]

I have two questions:

1. We want to add extension locator strategies to WebDriver, and one way to do that would be to register a chunk of JS with the driver and associate it with a custom locator name. Such an approach would share many characteristics with script pinning, and thus introduce a dependency here.

   My preference would be not to share the same infrastructure for extension locators as for script pinning because I think there are better ways (pre-registration through capabilities for example), but I wanted to make sure to raise the question that we all feel that way, and that extension selectors should not be implemented with pinned scripts?

2. Is there a use case to replace pinned scripts at runtime? If that is not a strong requirement, a simpler implementation would be to provide pinned scripts at session creation.

[jgraham]

I have two questions:

1. We want to add extension locator strategies to WebDriver, and one way to do that would be to register a chunk of JS with the driver and associate it with a custom locator name. Such an approach would share many characteristics with script pinning, and thus introduce a dependency here.
   My preference would be _not to share_ the same infrastructure for extension locators as for script pinning because I think there are better ways (pre-registration through capabilities for example), but I wanted to make sure to raise the question that we all feel that way, and that extension selectors should not be implemented with pinned scripts?

I think they should share the same infrastructure; they are basically the same feature but with the difference that the locator strategies must return either an element or an array of elements. I think that can work well with the approach in this PR.

2. Is there a use case to replace pinned scripts at runtime? If that is not a strong requirement, a simpler implementation would be to provide pinned scripts at session creation.

Only allowing pinned scripts to be uploaded at session creation seems interesting but I wonder if it's flexible enough. At session creation time you might not know that you're running e.g. react app tests which need a particular set of tests. With that in mind I wonder if it should be possible to remove one or all pinned scripts so you could pin some scripts in a fixture for a set of tests and unpin those scripts in the teardown for the tests without requiring the overhead of getting a new session between test modules.

[andreastt]

This matches the picture in my head of how script pinning should work, but we need to tighten up some terminology and the data structures.

[andreastt]

This is not true. Scripts injected via /session/{session id}/execute/sync and /session/{session id}/execute/async do not explicitly have an associated name. This should either be rearchitected so that you talk about pinned scripts (because they must always have an associated name) or allow for the executable script name to be optional and introduce a concept of anonymous executable scripts.

[titusfortner]

Right, this makes sense, let me think through the rewrite.

[titusfortner]

So, I was trying to get across the idea that the script itself is the same whether it is pinned or not. Rather than defining a separate "pinned script," it's just a matter of how you obtain it (either from pinned or from the parameter value). I'll change this to Each script may include an optional associated executable script name for now and we can address the bigger organizational consideration after I tidy things.

[andreastt]

How can a script be uniquely identified if it does not have an associated named?

[titusfortner]

It only needs a name if it is pinned, otherwise it is just processed from the provided arguments

[andreastt]

I guess this is correct, but I’m not sure it’s a particularly useful description. See my next comment.

[titusfortner]

yeah, my thought was to do what "Elements" section does, and try to define "Script" in the context of its usage in the main section, then have subsections that talk about what you do with them.

[andreastt]

If we can’t see a use case for getting the script, let’s avoid the complexity altogether.

I do see value in a driver refusing to pin a script for whatever reason deemed necessary.

[andreastt]

I think we should drop this requirement for pinning scripts as it can be done globally.

[titusfortner]

I do see value in a driver refusing to pin a script for whatever reason deemed necessary

Is this the same as the overwrite/error discussion, or is this just a failsafe for a driver to punt on things if it wants?

[andreastt]

I do see value in a driver refusing to pin a script for whatever reason deemed necessary

Is this the same as the overwrite/error discussion, or is this just a failsafe for a driver to punt on things if it wants?

Yes, this is specifically about the fact that something might happen on the driver side that might prevent it from storing the script. An example would be the driver running out of memory.

I thought we were all in agreement that another script pinned under the same name should overwrite the last one. At least I don’t think I suggested that.

[andreastt]

Only allowing pinned scripts to be uploaded at session creation seems interesting but I wonder if it's flexible enough. At session creation time you might not know that you're running e.g. react app tests which need a particular set of tests. With that in mind I wonder if it should be possible to remove one or all pinned scripts so you could pin some scripts in a fixture for a set of tests and unpin those scripts in the teardown for the tests without requiring the overhead of getting a new session between test modules.

This sounds reasonable to me.

I wonder if this means having an endpoint to clear the pinned scripts might be necessary. The way this proposal works the client would need to hold on to a reference to the pinned script and I wonder if this might be a problem for long-running sessions.

[titusfortner]

@twalpole this was actually your idea that I stole, chime in with your opinions, please.

[titusfortner]

@andreastt

1. We want to add extension locator strategies to WebDriver, and one way to do that would be to register a chunk of JS with the driver and associate it with a custom locator name. Such an approach would share many characteristics with script pinning, and thus introduce a dependency here.

I don't think we need something different for element location, because we already handle the use case of execute/sync endpoint returning an Element ID value.

All the client bindings need to be able to do is to allow the creation of custom locators. Like in Ruby I'd do something like:

# In Selenium bindings
Selenium::WebDriver::SearchContext::CUSTOM_FINDERS = []

def find_element(how, what)
  if FINDERS[how.to_sym]
    by = FINDERS[how.to_sym] 
    bridge.find_element_by by, what.to_s, ref
  elsif CUSTOM_FINDERS[how.to_sym]
    custom_proc = CUSTOM_FINDERS[how.to_sym]
    custom_proc.call(bridge, what)
   end
end

# In framework code (or user's test code)
def self.register_react_locator!
  driver.pin_script(:react_location, react_locate_script)
  Selenium::WebDriver::CUSTOM_FINDERS[:react] = Proc.new |context, args| { 
  context.execute_script(:react_location, *args)}
end

# In user's test code
MyTestFramework.register_react_locator!
driver.find_element(react_locate: 'MyComponent')

2. Is there a use case to replace pinned scripts at runtime? If that is not a strong requirement, a simpler implementation would be to provide pinned scripts at session creation.

Are you suggesting that we should only pin scripts at session creation? I'd like more flexibility than that.

[andreastt]

I don't think we need something different for element location, because we already handle the use case of execute/sync endpoint returning an Element ID value.

In my original comment I was actually suggesting we might need something different for custom locator strategies.

It is true you can work around this in the local end by calling a pinned script, but it would be more flexible and less dependent on a particular client implementation if the element retrieval commands would accept custom strategies in the shape of {"strategy": "react"}, where the react strategy is pre-defined in some way.

In any case, I’m glad we’ve established there’s no dependency here.

Are you suggesting that we should only pin scripts at session creation? I'd like more flexibility than that.

Some concrete use cases for script pinning at runtime were mentioned in #1445 (comment) and I said in #1445 (comment) that I was happy with the reasoning for that.

[titusfortner]

In my original comment I was actually suggesting we might need something different for custom locator strategies.

it would be more flexible and less dependent on a particular client implementation if the element retrieval commands would accept custom strategies

Yes, I agree the bindings need this. I think @shs96c said he had ideas related to this, but I don't know what he was considering. I like the lambda / closure approach I outlined above (which I think does what you are suggesting @andreastt), but I'm not sure how "Ruby" of an approach that is relative to what other languages can do.

[titusfortner]

So to summarize the various things that are still outstanding in the comments above.

I'm going to:

1. Change the names to not be part of the endpoints so that we can use colons 4a79a5d
2. Change the data structure to use Map instead of list & table accf942

Outstanding questions/decisions:

1. Do we want list / delete / delete all endpoints (my vote is no)
2. Do references to "script" need to be changed to "pinned script" for clarity? (I don't think so, but let me know)

Are there other outstanding issues to be addressed from the above that I'm glossing over?

[twalpole]

@titusfortner After a quick perusal of the comments here, for my use case I have no need for getting a list of the names, or for deleting scripts. Being able to overwrite an existing script would probably be useful though. I definitely wouldn't want a limit on the script size since the whole point of the feature is to replace large script transfers with much smaller calls. I'll try and read everything in more depth later today and may have some more useful comments.

[andreastt]

This needs to be something like:

A session has an associated map of executable scripts.

Then you need to add map to the Index under the Infra entry.

[andreastt]

This is already defined above under :2122.

Could you drop this paragraph and move the entry descriptions up?

(I think probably it would make more sense fo the various bit of session state to live closer to where they are used, but for the moment we should put it upstairs in the session chapter.)

[andreastt]

This is not a normative definition because it is never called anywhere. We only ever add something to this map when under the Pin Script command, so it would be sufficient to make this entire paragraph a step under this command.

[andreastt]

Similarly.

[andreastt]

Missing <p>.

[andreastt]

This should probably be:

<li>Return <a>executable script map</a>[<var>name</var>].

Or in a slightly longer form:

<li>Return the restult of <a>getting the value of an entry</a> on the <a>executable script map</a> given <var>name</var>.

[andreastt]

This is nitpicky, but I would prefer the slightly more English:

s/with argument/by/

[andreastt]

And somewhere in there (and below, under Execute Async Script) we need to replace the body with the pinned script, right?

[andreastt]

To be more in line with how Infra suggests using maps, both of these steps can be replaced with:

<li><p>Set <a>executable script map</a>[<var>name</var>] to <var>script</var>.

[andreastt]

Missing successful return here.

[andreastt]

@titusfortner

I think there is agreement on the general approach, but there are a number of outstanding issues that prevents this PR from landing.

Do you have time to move this forward?

I would like to wrap up all outstanding PRs before I proceed with #1462.

[titusfortner]

I'm... not certain I will have time this week. Maybe the weekend or early next week?

[christian-bromann]

This can be eventually replaced by the Bootstrap Scripts idea that was suggested for the BiDi proposal. This would solve the same use case of pinning scripts to a certain execution context (e.g. on a frame before page load) to attach functionality to the script context.

[shs96c]

I'm not sure that's true. A bootstrap script is run in every new instance of a context. The pinnable scripts are a shorthand for sending a single script to be executed right now across the wire: that is, the pinned scripts aren't available in every context, and so don't interfere with things like loading times or the performance of the browser.

The two could probably play nicely together: either choosing to update a bootstrap script using a pinned script, or calling something that needs to be on every page that's injected using a bootstrap script via a pinned script.

[whimboo]

Recently we landed the spec prose for Preload scripts in WebDriver BiDi. As such I'm fairly sure that we no longer want to add this feature to WebDriver classic. If we close as wontfix we should also close web-platform-tests/wpt#19675.

@jgraham do you have any objections?

[jgraham]

I agree that the main feature here is covered by preload scripts. The additional feature request at TPAC was the ability to use these as part of a locator strategy, but in theory middleware could already make that work (by intercepting the locator strategy and converting it into script execution).

[christian-bromann]

This is awesome, thanks folks!