feat: port image element plugin from main appium server #1
Conversation
|
CI seems to be unhappy |
|
this module needs a good README |
|
@jlipps What it your strategy about moving these parts out of the base driver, so we don't break anything in future Appium versions (both 1 and 2)? |
am working on it, this isn't ready to merge yet
Yes, will add one before merge
I am not going to move anything out of base driver for Appium 1.x. There is a branch in basedriver ( |
jlipps
force-pushed
the
images-plugin
branch
7 times, most recently
from
4501df6
to
c9902f1
Compare
|
Ok, CI is working, now all that remains is a good readme for the plug-in itself |
|
OK, all READMEs and appropriate docs have been added. This is ready to go IMO. |
| @@ -21,12 +25,14 @@ | |||
| }, | |||
| "homepage": "https://github.com/appium/appium-plugins#readme", | |||
| "dependencies": { | |||
| "@appium/base-plugin": "^1.5.0" | |||
| "@appium/base-plugin": "^1.5.0", | |||
There was a problem hiding this comment.
Not for this PR, but just I thought it was probably good to print author, homepage and license with --details option etc as part of the below. We'll also can guide reporters to check --details option to ensure where they can report issues. (or only homepage??)
e.g.
kazu$ appium driver list --details
✔ Listing available drivers
- xcuitest@3.31.5 [installed (NPM)], Appium <maintainers@appium.io>, https://github.com/appium/appium-plugins#readme, Apache-2.0
kazu$ appium plugin list
✔ Listing available plugins
There was a problem hiding this comment.
yes, that's a nice feature idea!
How about npm package versioning? If we bump the major version of base-driver package then this would mean we cannot do any major updates to it in Appium 1 anymore |
|
|
||
| The same goes for updating and removing the plugin using the CLI. | ||
|
|
||
| The `mainClass` value is the name that Appium should import from your project's entrypoint. So for example if somewhere in your project you have defined your plugin as follows: |
There was a problem hiding this comment.
Do we expect plugin authors to export multiple classes? If not we can just expect an ESM default export and can avoid having to define this.
WebdriverIO does this for reporters, e.g. if you define:
// wdio.conf.js
export.config = {
// ...
reporters: ['foobar']
// ...
}it will check first if there is a @wdio/foobar-reporter and if not imports wdio-foobar-reporter (so it prioritises project maintained plugins before community maintained ones).
It is slightly different with services as there are services that are loaded in the main process and others loaded in the worker process (and some in both). In the main process we try to import a named export launcher and check at the same time if there is also a default export we need to load in the worker. If not we ignore the service and save some memory as we don't require it in the worker process anymore.
I am not sure if we have for Appium plugins multiple types of version it would want to import. I can imagine different types of plugins here. One that enhances the driver with new commands, a different type for extending selector strategies etc. Plugins could be used as middlewares, e.g. before a command is send (to modify the payload) or after response has been received (to modify the response). Appium can define all these types and then expect plugin authors to have respective named exports for them.
There was a problem hiding this comment.
Do we expect plugin authors to export multiple classes? If not we can just expect an ESM default export and can avoid having to define this.
Plugins might export other things, or one package might export both a driver and a plugin, for example. I thought it best to require an explicit export, but we could also have a convention where if mainClass is not present, then the default export is used.
There was a problem hiding this comment.
Plugins could be used as middlewares, e.g. before a command is send (to modify the payload) or after response has been received (to modify the response). Appium can define all these types and then expect plugin authors to have respective named exports for them.
Yes, this is a good idea. However, in that case, the various exported plugin behaviors would not cohere as easily with one another. I thought a class-based approach was interesting because one instance of a plugin could have state shared between all its expressions (server updater, command handler, etc...)
packages/base/README.md
Outdated
| 1. Set the class variable `updatesServer` to `true`, so that Appium knows your plugin needs to update the server before launch. | ||
| 2. Implement the `async updateServer` method in your class. This method takes two parameters, `expressApp` and `httpServer`, which are the Express app object and the Node HTTP server object respectively. You can do whatever you want with them inside the `updateServer` method. |
There was a problem hiding this comment.
Wouldn't it be enough to check for an updateServer in the plugin prototype and if so assume the user likes to update the server?
There was a problem hiding this comment.
Appium could offer an automatism for this, e.g.:
export default class AppiumPlugin {
routes = [
['/some/command/route', this.commandA]
]
commandA (req, res) {
// ...
}
}Having the flexibility to work with the express objects is great no question but if Appium can simplify the most common use case it could make things easier!
There was a problem hiding this comment.
A question to be answered here is what happens if a command/endpoint is already defined? I assume right now it would just execute both command implementations but I can see scenarios where this is not a desired behavior or can cause confusion.
There was a problem hiding this comment.
Wouldn't it be enough to check for an updateServer in the plugin prototype and if so assume the user likes to update the server?
Not currently because the base plugin has updateServer defined, though granted this is primarily for the purpose of hosting docstrings.
There was a problem hiding this comment.
Having the flexibility to work with the express objects is great no question but if Appium can simplify the most common use case it could make things easier!
Implementing new commands doesn't require working with express objects, it involves handling it as part of handle. But I think you're right we could come up with a convention where we say a plugin handles a command if a method with that command name exists on the plugin instance. I think I like that. I'll probably implement it as a separate set of changes between this repo and basedriver.
There was a problem hiding this comment.
A question to be answered here is what happens if a command/endpoint is already defined? I assume right now it would just execute both command implementations but I can see scenarios where this is not a desired behavior or can cause confusion.
This is why we have next provided as part of handle. It allows a plugin to wrap the default behavior, or execute it after its own functionality. Here next encodes a whole chain of plugins that might act on a command. But it's up to each plugin to play nice, because we can't assume that plugins are designed to work with one another. If the goal of plugin A is to completely replace the functionality of a command, and the goal of plugin B is the same, then whichever plugin is loaded first is the one which will handle it, and the other won't.
This does mean that plugins are not necessarily compatible with one another and users might need to be aware of this. But I can't think of any meaningful way for us to force plugins to play nice with one another since the point is for them to have a lot of power.
The Appium 2.0 branch of basedriver won't be merged until Appium 2.0 is ready to go. I don't think we're planning on supporting Appium 1.x with major changes to basedriver alongside Appium 2.x work, right? |
all of this functionality is going to be a plugin. there will be another PR that will remove the related code from basedriver.