ui5-community / wdi5

official UI5 end-to-end test framework for UI5 web-apps. wdi5 = Webdriver.IO + UI5 Test API
https://ui5-community.github.io/wdi5/
Apache License 2.0
102 stars 43 forks source link

Ambient types of `wdi5` not working (wdi5 v2, esm, typescript) #573

Closed LukasHeimann closed 8 months ago

LukasHeimann commented 8 months ago

Describe the bug In my project, we just migrated to wdi5 v2. We are using TypeScript, and we also migrated to ES Modules as target (instead of CommonJS scripts). For this setup (which is supported by wdi5), the ambient types that wdi5 provides are broken. While browser.asControl() is available as global type as expected, neither its parameter nor its return value is typed. It seems like in an actual project, the types referenced there can't be resolved and thus default to any, which causes problems.

To Reproduce https://github.com/LukasHeimann/wdi5-types-sample

Expected behavior browser.asControl() is typed correctly

Runtime Env (please complete the following information):

Additional context I guess this may be due to improper bundling for esm. When you have esm, your imports must end with the file ending .js. This is even true in TypeScript projects as wdi5 is one: https://github.com/microsoft/TypeScript/issues/16577. However it seems like this was not migrated, and so imports are failing if esm is used. Compare also built-in wdio plugins that do this correctly: https://github.com/webdriverio/webdriverio/blob/main/packages/wdio-reporter/src/index.ts

For bundling commonjs, I guess you'd need to then strip the .js extension anymore, though I am not sure about that.

Siolto commented 8 months ago

Hi @LukasHeimann,

in your tsconfig.json file try: wdio-ui5-service/esm.

LukasHeimann commented 8 months ago

Did that for the MVP linked above, but that gives an error:

error TS2688: Cannot find type definition file for 'wdio-ui5-service/esm'.
  The file is in the program because:
    Entry point of type library 'wdio-ui5-service/esm' specified in compilerOptions

  tsconfig.json:13:7
    13       "wdio-ui5-service/esm"
             ~~~~~~~~~~~~~~~~~~~~~~
    File is entry point of type library specified here.
LukasHeimann commented 8 months ago

I think the ambient types are available in principle (asControl is defined), but the imports in https://github.com/ui5-community/wdi5/blob/main/src/types/browser-commands.ts don't seem to be working.

Siolto commented 8 months ago

This adapted tsconfig.json is working for me with your MVP:

{
  "compilerOptions": {
    "moduleResolution": "node",
    "strict": true,
    "types": [
      "node",
      "@sapui5/types",
      "@wdio/globals/types",
      "expect-webdriverio",
      "@wdio/mocha-framework",
      "wdio-ui5-service/esm"
    ],
    "skipLibCheck": true
  }
}

need to check why your config throws errors.

LukasHeimann commented 8 months ago

Of course typescript will not complain about esm issues if you disable esm πŸ˜‰

https://www.typescriptlang.org/tsconfig#module The defafult option is not using the esmodule support of node, as recommended in typescript: https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping

Small addendum: https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext explains it quite nicely:

A common misconception is that node16 and nodenext only emit ES modules. In reality, node16 and nodenext describe versions of Node.js that support ES modules, not just projects that use ES modules. [...] they are the only correct module options for all apps and libraries that are intended to run in Node.js v12 or later, whether they use ES modules or not.

Not using Node16 modules would probably cause a fallback to another moduleResolution as well. I'm not sure if node10 is taken then, but that would, e.g., disregard the exports setting in the package.json, which is why adding wdio-ui5-service/esm works when not using the right moduleResolution (https://www.typescriptlang.org/docs/handbook/modules/reference.html#supported-features-2)

LukasHeimann commented 8 months ago

I guess this problem isn't too big, as most imports were already adjusted to use the .js suffix -- it's just missing for type declarations, which is probably why this isn't causing issues at runtime.

I've drafted up https://github.com/ui5-community/wdi5/compare/main...LukasHeimann:wdi5:pathModuleConfiguration, let me know if I should open a PR. I haven't executed any tests besides making sure that the npm run build still passes (which is the typecheck, basically)

vobu commented 8 months ago

PR please, heck yeah!

LukasHeimann commented 8 months ago

That probably was a bit too fast -- while that patch was reasonable, it wasn't complete... I was currently trying to get npm link to work when you merged this, but the problem is still there in the newly released version... Sorry

vobu commented 8 months ago

the pitfalls of relying on extensive CI ... 😐 then I'll wait for both your new PR and your explicit OK/review request before doing so...

LukasHeimann commented 8 months ago

So, I had a couple of minutes to look into it. 2.0.5 has fixed the issue with the ambient types, so this issue being closed is totally fine indeed.

In my actual project, there was another problem revolving around importing the type wdi5Control, which was giving eslint typescript a hard time. I think I have figured that out now and will raise a new PR.

LukasHeimann commented 8 months ago

I've created https://github.com/ui5-community/wdi5/pull/576 -- this time fully tested against my local project with npm link, where the issues are gone after this πŸŽ‰