fix(build): avoid build failures and rejigger types

[boneskull]

I could not build this project locally, so I set out to fix it. I think I did? There were problems especially with wdio, and I know from experience that ts-node is required to address them. I have also added @types/mocha but we're not actually doing much typechecking.

Anyway, we now have incremental builds! That means "it's faster" since TS will only build one project at a time instead of all of them.

I was able to remove build.js, since tsc -b does the same thing now. I was unable to retain the index.d.ts in the base plugin (though could probably figure it out if I spent more time with it); instead, the definitions are now in the docstrings of plugin.js. All declarations are now generated by tsc and are output alongside the output .js files. Please note that I did have to actually implement a couple functions which were previously commented-out. We will want to define a Plugin interface later which allows the <cmd> methods. Since these don't exist in BasePlugin, we should not attempt to define them in this type.

Not too much actual type-checking is happening here, but we can enable that later (per file via // @ts-check pragma, or a checkJs: true in a project's tsconfig.json).

[boneskull]

OK, I think I need to revert a portion of this for now. I'm still unsure how, exactly, to get the generated .d.ts files to read the hand-edited ones... hmm.

[jlipps]

I don't actually see any type files in this PR, are they not meant to be added?

[boneskull]

I don't actually see any type files in this PR, are they not meant to be added?

No, they are all just build artifacts. But as I mentioned above (somewhere), I don't think I can get away with that.

[boneskull]

I like the idea of making BasePlugin a pure interface rather than an actual class, but we make use of class/interface-specific features like static fields that would be hard to model outside of a real interface model (i.e., duck typing would be not ideal).

I'm not sure I follow--is this in reference to this comment?

If we don't have classes, we don't need static members. e.g.,

/** @type {IPlugin} */
const myPlugin = {
  name: 'MyPlugin',
  newMethodMap: { /* stuff */},
  async updateServer(app, server) {
    /* stuff */
  },
  findElement(context, driver, next, strategy, selector) { 
    // `context` is what `this` would have been and is of type `Plugin`.  
    // interface `Plugin` extends `IPlugin`, but it would have props `logger`, `name`, and `opts`.
    // when Appium loads `myPlugin`, it could use something like `Object.create({...myPlugin, logger, etc})`
    // to create a `Plugin`.  
    // additionally, we could merge interface `Plugin` with _class_ `Plugin` (currently `BasePlugin`) in case
    // somebody really wanted to do `class MyPlugin extends Plugin`
  }
}
export default myPlugin;

but, as I said, I understand if you don't want to change the API to deviate from how drivers work (unless you wanted to change that as well). it'd be an opportune time to do so, however!

If we change the API or not, we still have three different types we need to separate:

1. the interface that extensions must implement (call it IPlugin)
2. some default behavior (e.g., the BasePlugin class; mainly just its constructor)
3. the result of applying BasePlugin to an IPlugin, which results in a working instance of type e.g., Plugin

[boneskull]

the result of applying BasePlugin to an IPlugin, which results in a working instance of type e.g., Plugin

That means that this in a method of an IPlugin is not an IPlugin but rather a Plugin, if you follow.

[jlipps]

Before I was just trying to say that in Appium we currently make use of both the class-level fields and the instance-level fields. I.e., we distinguish between class objects and instances, so if we merge the concept of a plugin into a single object that implements an interface, we need to account for that in a redesign of how we work with plugins in the main Appium server code.

[boneskull]

oh..yeah, I was assuming that to support anything new we'd have to write code lol

[jlipps]

My vote would be to leave as-is right now, and for Appium 3 we could target updating the driver/plugin mode to be interfaces if we want (I'm not actually sure it will ever make reasonable sense to change drivers from instances to objects satisfying interfaces), hopefully requiring minimal updates on part of the extension authors. Right now would be an especially bad time to try and update driver handling since we need to support old appium 1.x drivers with appium 2.0, so there's probably lots of challenges if any driver updates are required.

[boneskull]

Fair enough. I'll close this PR and re-open one with minimal changes to just to get the build running. We will still need to consider those three concepts, but it doesn't have to happen right away

[jlipps]

Yes, please reopen one with all your build modifications. I had to steal from them in a branch where I'm adding a new plugin.

[jlipps]

@boneskull are you able to open your minimal PR soon? I have work i want to do that is sort of reimplementing what you've done but you've done it more cleanly i imagine.

[boneskull]

should be able to work on this today

[boneskull]

@jlipps OK, after pulling master I am now able to build properly, so I have no idea what's going on here. I am not going to change anything right now, so you should go ahead and do what you're going to do.

[boneskull]

(I'm unsure how to make incremental builds work properly with a hand-coded .d.ts file. If you really want that, I can spend some more time trying to figure it out, otherwise I'd just as soon leave it as-is.)

[jlipps]

harrumph. ok. i think my problem is i added webdriverio as a dep and i got tons of ts build errors until i applied some of your magic changes, without understanding what i was doing.

[boneskull]

to be sure, do not assume I know what I'm doing either. it's mostly just adapting config from some rando's tutorial.