Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
LukasHeimann opened this issue Jan 10, 2024 · 11 comments
Closed

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

LukasHeimann opened this issue Jan 10, 2024 · 11 comments

Comments

@LukasHeimann
Copy link
Contributor

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):

  • wdi5/wdio-ui5-service-version: 2.0.4
  • UI5 version: 1.120.4
  • wdio-version (output of wdio --version): 8.27.1
  • node-version (output of node --version): 20.9.0
  • OS: windows
  • Browser + Version: n/a

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: microsoft/TypeScript#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
Copy link
Collaborator

Siolto commented Jan 10, 2024

Hi @LukasHeimann,

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

@LukasHeimann
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

Siolto commented Jan 10, 2024

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
Copy link
Contributor Author

LukasHeimann commented Jan 10, 2024

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
Copy link
Contributor Author

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 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
Copy link
Contributor

vobu commented Jan 10, 2024

PR please, heck yeah!

@LukasHeimann
Copy link
Contributor Author

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
Copy link
Contributor

vobu commented Jan 10, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

I've created #576 -- this time fully tested against my local project with npm link, where the issues are gone after this 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants