Tracking doc: rewrite `@wdio/sauce-service` package into TypeScript #6001

martinfrancois · 2020-10-17T16:51:50Z

Proposed changes

Rewrite @wdio/sauce-service package into TypeScript, see #5843

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Checklist

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

packages/wdio-sauce-service/src/launcher.ts

packages/wdio-sauce-service/tests/service.test.ts

packages/wdio-utils/package.json

packages/wdio-utils/utils.d.ts

martinfrancois · 2020-10-21T22:04:44Z

After rebasing I still get the error:

error TS2688: Cannot find type definition file for '@wdio/utils'.

(see builds here)

Any idea @christian-bromann ?

martinfrancois · 2020-10-21T22:23:03Z

@christian-bromann I think it could be because of the order in which we build the typescript files here, as generated by ./scripts/build:

npx tsc -b packages/devtools/tsconfig.json packages/wdio-allure-reporter/tsconfig.json packages/wdio-appium-service/tsconfig.json packages/wdio-applitools-service/tsconfig.json packages/wdio-browserstack-service/tsconfig.json packages/wdio-cli/tsconfig.json packages/wdio-concise-reporter/tsconfig.json packages/wdio-config/tsconfig.json packages/wdio-crossbrowsertesting-service/tsconfig.json packages/wdio-cucumber-framework/tsconfig.json packages/wdio-devtools-service/tsconfig.json packages/wdio-dot-reporter/tsconfig.json packages/wdio-firefox-profile-service/tsconfig.json packages/wdio-jasmine-framework/tsconfig.json packages/wdio-junit-reporter/tsconfig.json packages/wdio-local-runner/tsconfig.json packages/wdio-logger/tsconfig.json packages/wdio-mocha-framework/tsconfig.json packages/wdio-repl/tsconfig.json packages/wdio-reporter/tsconfig.json packages/wdio-runner/tsconfig.json packages/wdio-sauce-service/tsconfig.json packages/wdio-selenium-standalone-service/tsconfig.json packages/wdio-shared-store-service/tsconfig.json packages/wdio-smoke-test-reporter/tsconfig.json packages/wdio-smoke-test-service/tsconfig.json packages/wdio-spec-reporter/tsconfig.json packages/wdio-static-server-service/tsconfig.json packages/wdio-sumologic-reporter/tsconfig.json packages/wdio-sync/tsconfig.json packages/wdio-testingbot-service/tsconfig.json packages/wdio-utils/tsconfig.json packages/wdio-webdriver-mock-service/tsconfig.json packages/webdriver/tsconfig.json packages/webdriverio/tsconfig.json

At least I noticed if I run the command npm run setup-full first, then when it fails I run node ./scripts/build I don't get the error anymore. So I suspect it could be that since wdio-utils is built AFTER wdio-sauce-service is built, but since wdio-sauce-service depends on wdio-utils, it throws an error since it doesn't have the types.

martinfrancois · 2020-10-21T23:19:54Z

@christian-bromann sorry for the spam, but I now added a commit (79201d3) where I made it possible to define packages that should be compiled first, to avoid the issue I described in #6001 (comment)

Should I open up a separate PR for that or should we keep it in this one?

packages/wdio-sauce-service/src/launcher.ts

packages/wdio-sauce-service/src/utils.ts

packages/wdio-sauce-service/tests/service.test.ts

christian-bromann · 2020-10-22T07:11:36Z

Good catch with the compile order. This could complain some other side issues I experienced.

martinfrancois · 2020-10-23T21:10:24Z

@christian-bromann just resolved your comments, thanks!

packages/wdio-sauce-service/src/service.ts

christian-bromann · 2020-10-25T18:31:45Z

packages/wdio-sauce-service/sauce-service.d.ts

@@ -1,151 +0,0 @@
-declare module WebdriverIO {


we can't remove this file as the package.json points to it https://github.com/webdriverio/webdriverio/pull/6001/files#diff-1e974f46805437badbcd51209a7a5b716c7c08ec4b9c1554ffcbc9ca9fa48ef3R45

I didn't remove it though - only its contents, and I did so since they are outdated (and now included in node-saucelabs, do we need to do anything differently here?

let's keep this file for now because it is still being referred to in the package.json ("types": "sauce-service.d.ts")

packages/wdio-sauce-service/src/service.ts

…st` into interface `TestResult`

martinfrancois · 2020-10-27T01:05:38Z

@christian-bromann can you have a look again please? Seems like only one question from you that I answered is left.

christian-bromann · 2020-10-27T07:27:19Z

packages/wdio-sauce-service/package.json

+  "devDependencies": {
+    "webdriverio": "6.7.0",
+    "webdriver": "6.7.0",
+    "@wdio/cucumber-framework": "6.7.0",
+    "@wdio/jasmine-framework": "6.7.0",
+    "@wdio/reporter": "6.7.0",
+    "@types/cucumber": "6.0.1",
+    "cucumber": "6.0.5"
+  },


This will be problematic if we release. As we are in a production environment it won't install dev dependencies and therefor the compile step will fail. I would suggest:

remove cucumber

move @types/cucumber into the dependencies

reference all wdio types through tsconfig.json and use them through their namespace, e.g. WebDriver.DesiredCapabilities rather than importing them.

WDYT?

Sounds good, I'll try it and see if it works and report back if I run into issues.

@christian-bromann by reference all wdio types through tsconfig.json you did mean to do it like that, right?:

"types": ["webdriverio", "webdriver", "@wdio/jasmine-framework" ,"@wdio/cucumber-framework", "@wdio/mocha-framework", "@wdio/utils"]

Or did you mean something else?

Because I'm getting an error, see launcher.ts:21:

TS2503: Cannot find namespace 'Webdriver'. TS4063: Parameter 'capabilities' of constructor from exported class has or is using private name 'Webdriver'.

ok, I guess this solution doesn't work then and we have to use references like Jest does.

I googled a bit and came to the same conclusion, I also found this:
https://medium.com/@NiGhTTraX/how-to-set-up-a-typescript-monorepo-with-lerna-c6acda7d4559
https://github.com/NiGhTTraX/ts-monorepo

In this case, I guess we need to change the root tsconfig.json to the following, right?:

{ "extends": "./tsconfig.prod.json", "compilerOptions": { "inlineSourceMap": true, "baseUrl": ".", "paths": { "@wdio/*": ["packages/wdio-*/src"], "devtools": ["packages/devtools/src"], "eslint-plugin-wdio": ["packages/eslint-plugin-wdio/src"], "webdriver": ["packages/webdriver/src"], "werbdriverio": ["packages/webdriverio/src"] } } }

If the build command runs fine, does this indicate it will work in production as well?

Did you try specifying types in package.json? I hope this can solve typescript monorepo problem
https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#including-declarations-in-your-npm-package

@hieuxlu the source code is right there for you to check yourself, but yes I did so ;)

hieuxlu · 2020-10-27T15:58:07Z

packages/wdio-sauce-service/src/service.ts

+interface MultiRemoteBrowser extends WebdriverIO.BrowserObject {
+    instances: string[]
+    isMultiremote: boolean
+}


This is not reusable for other services @christian-bromann @martinfrancois. We have browserstack service, testingbot services eagerly waiting to use multiremote browser types.

do they have the same kind of type annotation? If so, we could think about extracting it somewhere more global, but anyways we could always move this in a later PR into a different package when there is the need for it.

Yes its common properties in global browser object, but not define yet. And yes, I agree it could be a separate issue. I just wonder if you guys could think of any better solution?

I personally like not to prematurely optimize things - my preferred approach is usually to adapt things to be as generic as they need to be, once it's needed. So I would rather start with having this interface here (as it's the only place it's used right now) and refactoring it to a different place as soon as the need for it arises. And where to put it then will be something to look at specifically in the context. Especially considering we are not exporting this type, so we don't run risk of any breaking changes when we move this to a different place.

hieuxlu · 2020-10-27T16:00:15Z

packages/wdio-sauce-service/src/service.ts

-        const browserName = global.browser.instances.filter(
-            (browserName) => global.browser[browserName].sessionId === newSessionId)[0]
+        const browserName = this.multiRemoteBrowser.instances.filter(
+            (browserName: string) => (global.browser[browserName as keyof BrowserObject] as BrowserObject).sessionId === newSessionId)[0]


I would say this as quite hacky @christian-bromann. browserName is definitely not in the list of keyof BrowserObject

Since the type of global.browser is BrowserObject, and you are trying to access a key from global.browser here, what else could it be other than a key of BrowserObject?

I think this is the case of accessing something like Record<string, BrowserObject>. If you use <keyOf>, the property will be casted to any, so the typecheck would provide no value.
So using global.browser[browserName as keyof BrowserObject] is equivalent to (global.browser as any)[browserName]

If you use <keyOf>, the property will be casted to any, so the typecheck would provide no value.

That's actually incorrect, have a look at the following example:

const myObj = { Hello: "world" }; type ObjKey = keyof typeof myObj const foo: ObjKey = "foo";

If what you are saying was correct, that keyof would cast it to any, then I would expect this example to compile without any errors (since ObjKey would be any). However, this results in the following error:

Initializer type "foo" is not assignable to variable type ObjKey

If I change the last line to be const foo: ObjKey = "Hello";, it compiles fine without any errors.
So clearly TypeScript preserves the types when using keyof.

But you are right in a way that by using browserName as keyof BrowserObject we are implicitly telling TypeScript to assume that browserName is a key of BrowserObject, even though it comes from this.multiRemoteBrowser.instances (which is a plain array of strings, with no guarantee of those strings being keyof BrowserObject). To be 100% correct we would need to either type instance as (keyof BrowserObject)[] and or run checks on runtime to ensure that instances are indeed keys of BrowserObject. But this would limit the instances that can be used, which could get annoying to maintain (as the code needs to be changed for each new type of instance) and it would most likely be overkill here and the risk of breaking things would be much higher than the benefit of the type safety we would gain in this situation, in my opinion.

Considering the code has been running fine previously, I'm quite hesitant with adding any more checks here, we could always consider improving type safety here in the future, but probably this would be a bigger undertaking. Of course I could be totally missing something, in which case I would be glad if you could point it out or make suggestions on how to improve the situation.

…ve `cucumber` Signed-off-by: martinfrancois <f.martin@fastmail.com>

Signed-off-by: martinfrancois <f.martin@fastmail.com>

christian-bromann · 2020-10-27T21:41:55Z

@martinfrancois there still seem to be some legit compiling errors:

Error: packages/wdio-sauce-service/src/launcher.ts(21,62): error TS2503: Cannot find namespace 'Webdriver'.
Error: packages/wdio-sauce-service/src/launcher.ts(21,100): error TS2503: Cannot find namespace 'Webdriver'.
Error: packages/wdio-sauce-service/src/launcher.ts(29,68): error TS2304: Cannot find name 'DesiredCapabilities'.

martinfrancois · 2020-10-27T21:43:05Z

@christian-bromann yes, I mentioned the reason for them here: #6001 (comment)

christian-bromann · 2020-11-16T11:10:19Z

@martinfrancois can we update the branch as have added a bunch of more rewrites to master.

I think the following way of handling external types works best: add webdriverio or any other sub package to the "types" properties in the subpackage tsconfig:

e.g. in packages/devtools/tsconfig.json:

{
    "extends": "../../tsconfig",
    "compilerOptions": {
        "baseUrl": ".",
        "outDir": "./build",
        "rootDir": "./src",
        "types": ["webdriverio", "@wdio/protocols"],
    },
    "include": [
        "src/**/*"
    ]
}

Then you can use the types in all files via, e.g. in packages/devtools/src/index.ts

static async newSession (options: WebDriver.Options = {}, modifier?: Function, userPrototype = {}, customCommandWrapper?: Function) {
        const params: WebDriver.Options = validateConfig(DEFAULTS, options)

Note: the difference between Webdriver (wrong namespace) and WebDriver (correct namespace).

Happy to help in any way to get this landed.

christian-bromann · 2020-12-03T06:08:50Z

@martinfrancois do you plan to continue working on this?

martinfrancois · 2020-12-05T17:06:01Z

@christian-bromann excuse me, didn't notice your update here: #6001 (comment)

Yes, I'll try your suggestion and will get back to you!

christian-bromann · 2020-12-29T00:30:31Z

@martinfrancois we are aiming for a v7 release by end of January. Do you think you can push this over the line until then? If not, no problem. I am happy to pick this one up.

christian-bromann · 2020-12-29T08:33:02Z

I will go ahead and take this over. I will use what you have done so far.

christian-bromann · 2020-12-29T11:44:47Z

Closing in favor of #6274

martinfrancois self-assigned this Oct 17, 2020

martinfrancois added Tracking 🗒 Tracking issues TypeScript wdio-sauce-service labels Oct 17, 2020

christian-bromann reviewed Oct 19, 2020

View reviewed changes

martinfrancois force-pushed the 5843-ts-sauce-service branch 3 times, most recently from f8db4f8 to fd4258c Compare October 21, 2020 21:56

martinfrancois force-pushed the 5843-ts-sauce-service branch 2 times, most recently from cd0aefb to 79201d3 Compare October 21, 2020 23:16

christian-bromann reviewed Oct 22, 2020

View reviewed changes

packages/wdio-sauce-service/src/launcher.ts Outdated Show resolved Hide resolved

packages/wdio-sauce-service/src/utils.ts Outdated Show resolved Hide resolved

packages/wdio-sauce-service/tests/service.test.ts Outdated Show resolved Hide resolved

martinfrancois force-pushed the 5843-ts-sauce-service branch 2 times, most recently from 8304374 to 80e0845 Compare October 23, 2020 21:09

martinfrancois requested a review from christian-bromann October 23, 2020 21:10

martinfrancois force-pushed the 5843-ts-sauce-service branch 4 times, most recently from cff79b2 to 2431b46 Compare October 24, 2020 15:52

christian-bromann reviewed Oct 25, 2020

View reviewed changes

martinfrancois force-pushed the 5843-ts-sauce-service branch from 6f88f90 to 7336e59 Compare October 26, 2020 12:42

martinfrancois added 3 commits October 26, 2020 13:42

wdio-sauce-service: rename *.js to *.ts

6c03393

wdio-sauce-service: update saucelabs dependency to 4.5.1

54cb5ac

webdriverio: extract result parameter from afterHook and `afterTe…

ae84856

…st` into interface `TestResult`

martinfrancois force-pushed the 5843-ts-sauce-service branch 2 times, most recently from 10f6498 to 979d246 Compare October 27, 2020 00:49

wdio-sauce-service: migrate to typescript

a2ccb3a

martinfrancois force-pushed the 5843-ts-sauce-service branch from 979d246 to a2ccb3a Compare October 27, 2020 00:54

martinfrancois requested a review from christian-bromann October 27, 2020 01:05

christian-bromann reviewed Oct 27, 2020

View reviewed changes

christian-bromann mentioned this pull request Oct 27, 2020

Tracking doc: rewrite @wdio/testingbot-service package into TypeScript #6032

Merged

8 tasks

hieuxlu reviewed Oct 27, 2020

View reviewed changes

martinfrancois added 2 commits October 27, 2020 21:59

wdio-sauce-service: move @types/cucumber to dependencies and remo…

5ae7ca8

…ve `cucumber` Signed-off-by: martinfrancois <f.martin@fastmail.com>

wdio-sauce-service: get DesiredCapabilities from namespace

73e8b67

Signed-off-by: martinfrancois <f.martin@fastmail.com>

martinfrancois requested review from christian-bromann and hieuxlu October 27, 2020 21:40

christian-bromann mentioned this pull request Oct 29, 2020

Tracking doc: rewrite @wdio/sauce-service package into TypeScript #5843

Closed

christian-bromann closed this Dec 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracking doc: rewrite `@wdio/sauce-service` package into TypeScript #6001

Tracking doc: rewrite `@wdio/sauce-service` package into TypeScript #6001

martinfrancois commented Oct 17, 2020

martinfrancois commented Oct 21, 2020

martinfrancois commented Oct 21, 2020

martinfrancois commented Oct 21, 2020 •

edited

christian-bromann commented Oct 22, 2020

martinfrancois commented Oct 23, 2020

christian-bromann Oct 25, 2020

martinfrancois Oct 26, 2020

christian-bromann Oct 27, 2020

martinfrancois commented Oct 27, 2020

christian-bromann Oct 27, 2020

martinfrancois Oct 27, 2020 •

edited

martinfrancois Oct 27, 2020

christian-bromann Oct 27, 2020

martinfrancois Oct 27, 2020

hieuxlu Oct 28, 2020

martinfrancois Oct 28, 2020

hieuxlu Oct 27, 2020

martinfrancois Oct 27, 2020

hieuxlu Oct 27, 2020 •

edited

martinfrancois Oct 29, 2020

hieuxlu Oct 27, 2020

martinfrancois Oct 27, 2020 •

edited

hieuxlu Oct 27, 2020 •

edited

martinfrancois Oct 28, 2020 •

edited

christian-bromann commented Oct 27, 2020

martinfrancois commented Oct 27, 2020

christian-bromann commented Nov 16, 2020

christian-bromann commented Dec 3, 2020

martinfrancois commented Dec 5, 2020

christian-bromann commented Dec 29, 2020

christian-bromann commented Dec 29, 2020

christian-bromann commented Dec 29, 2020

Tracking doc: rewrite @wdio/sauce-service package into TypeScript #6001

Tracking doc: rewrite @wdio/sauce-service package into TypeScript #6001

Conversation

martinfrancois commented Oct 17, 2020

Proposed changes

Types of changes

Checklist

Further comments

Reviewers: @webdriverio/project-committers

martinfrancois commented Oct 21, 2020

martinfrancois commented Oct 21, 2020

martinfrancois commented Oct 21, 2020 • edited

christian-bromann commented Oct 22, 2020

martinfrancois commented Oct 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinfrancois commented Oct 27, 2020

Choose a reason for hiding this comment

martinfrancois Oct 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieuxlu Oct 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinfrancois Oct 27, 2020 • edited

Choose a reason for hiding this comment

hieuxlu Oct 27, 2020 • edited

Choose a reason for hiding this comment

martinfrancois Oct 28, 2020 • edited

Choose a reason for hiding this comment

christian-bromann commented Oct 27, 2020

martinfrancois commented Oct 27, 2020

christian-bromann commented Nov 16, 2020

christian-bromann commented Dec 3, 2020

martinfrancois commented Dec 5, 2020

christian-bromann commented Dec 29, 2020

christian-bromann commented Dec 29, 2020

christian-bromann commented Dec 29, 2020

Tracking doc: rewrite `@wdio/sauce-service` package into TypeScript #6001

Tracking doc: rewrite `@wdio/sauce-service` package into TypeScript #6001

martinfrancois commented Oct 21, 2020 •

edited

martinfrancois Oct 27, 2020 •

edited

hieuxlu Oct 27, 2020 •

edited

martinfrancois Oct 27, 2020 •

edited

hieuxlu Oct 27, 2020 •

edited

martinfrancois Oct 28, 2020 •

edited