SAP / ui5-typescript

Tooling to enable TypeScript support in SAPUI5/OpenUI5 projects
https://sap.github.io/ui5-typescript
Apache License 2.0
200 stars 28 forks source link

QUnit not recognized when using @sapui5/ts-types[-esm] #408

Closed sap-sebelao closed 1 year ago

sap-sebelao commented 1 year ago

Describe the bug I am getting the following error whenever I attempt to use QUnit while using the https://www.npmjs.com/package/@sapui5/ts-types-esm package for types image

I have an internal example than I can provide, but this can also be reproduced using https://github.com/SAP-samples/ui5-typescript-helloworld/tree/testing

1) clone https://github.com/SAP-samples/ui5-typescript-helloworld and checkout the branch testing 2) package.json : remove @types/openui5, replace with @sapui5/ts-types-esm@1.116.0 or @sapui5/ts-types@1.116.0 3) tsconfig.json now needs to be fixed with "types": ["@sapui5/ts-types-esm"] 4) open webapp/test/unit/controller/App.qunit.ts

Actually happens in many other files where QUnit global is used.

Expected behavior QUnit should be recognized the same as when using @types/openui5

Additional context As mentioned above, this happens in the sapui5 type packages, including the -esm version, but does not happen at all in the openui5 one.

Not sure if this is user error, but I did not find this difference mentioned anywhere, so I expected the packages to work similarly in this case. Sorry if I missed a memo somewhere.

akudev commented 1 year ago

As soon as you add "types": ["@sapui5/ts-types-esm"] to the tsconfig, you make TypeScript ignore any types except for these when calculating the global scope - see https://www.typescriptlang.org/tsconfig#types This means that QUnit is no longer available globally without importing.

One solution is to also add the QUnit types ("types": ["@sapui5/ts-types-esm", "@types/qunit"]) like it is e.g. done in the Easy UI5 template. In samples using the @types/openui5 the entire types entry is unneeded, hence also the addition of @types/qunit cannot be demonstrated.

Another option is to add @sapui5 to the list of roots for type directories by adding the following line instead of setting "types":

 "typeRoots": ["./node_modules/@sapui5", "./node_modules/@types"]

But this may lead to errors once any non-type folders end up below ./node_modules/@sapui5, e.g. in case we create new packages in this namespace and you consume them in the application. (Not planned right now, but who knows.)

A third option would be not to rely on the global presence of QUnit but to import it explicitly where it is used (this is possible with the statement import QUnit from "sap/ui/thirdparty/qunit-2"; since version 1.112, as this is since when we declare this module in the type definitions - see the release notes). However, this import will fail with Uncaught Error: QUnit has already been defined. in case QUnit had been loaded before without the UI5 module loading being aware - e.g. via <script> tag in the HTML page, as it is currently typically done in our samples. This means for this third option one would have to get rid of this script include (and any others depending on it) and explicitly import QUnit wherever it is used.

All in all, the first option is recommended right now, but one has to be aware that any further types need to be added to the list to be available globally.

akudev commented 1 year ago

Oh, and please use @sapui5/types instead of @sapui5/ts-types-esm in version 1.113 and higher!

sap-sebelao commented 1 year ago

I am sorry, I should've realized that types makes TS calculate global scope differently.

I actually fixed it in the meantime exactly the way you described, although it seems TS recognized it as a different QUnit version than what SAPUI5 actually uses. I am guessing there is another user error somewhere, but I was able to make it work after some adjustments at least.

Thanks for the recommendation about @sapui5/types, I definitely have to try that. The internal template has generated with ts-types-esm and we did not feel the need to change it.

akudev commented 1 year ago

The need to change away from "ts-types-esm" will arise exactly when we stop publishing further updates for that package. ;-) Until then, they are identical.

Does your package.json also require the qunit types? The SAPUI5 types already do that - and reference the closest possible version: not all versions are available, so it's not exactly the one coming with the runtime, regardless of whether the SAPUI5 dependency is used or you have overridden it.

sap-sebelao commented 1 year ago

Does your package.json also require the qunit types? The SAPUI5 types already do that - and reference the closest possible version: not all versions are available, so it's not exactly the one coming with the runtime, regardless of whether the SAPUI5 dependency is used or you have overridden it.

That's a good point, I did put @types/qunit into dev dependencies, along with @types/sinon, so I've probably overriden it with a much newer version.

After removing both, I still got errors, which was strange. I tried replacing @sapui/ts-types-esm with @sapui5/types and voila, QUnit works fine!

However, sinon is still not recognized by TS. Is sinon getting the same treatment by SAPUI5 types?

akudev commented 1 year ago

I still got errors, which was strange

This might be related to the qunit types being present twice, once in the toplevel node_modules folder because you required them, once below the UI5 types (in .node_modules/@sapui5/types/node_modules/@types/qunit). Even when your direct dependency is removed, npm does not move the types up to their standard location, hence the tsconfig settings do not allow them being found. Only when you removed the ts-types-tsm, npm did newly add the qunit types right at toplevel.

Is sinon getting the same treatment by SAPUI5 types?

No, as the UI5 types do not use any sinon APIs (i.e. no UI5 method returns or requires a type from sinon), our types do not declare sinon to be a dependency. This means you need to declare the sinon types yourself as dependency and in case there are any globals from them, you also need to add these types to the "types": list. That's the drawback of using "types":, as I have now described in my multiple-times-edited initial reply above, which I now think overs all relevant options.

sap-sebelao commented 1 year ago

Thanks for all the very helpful answers!

EDIT: for anyone stumbling into this, who may be interested by solution for the sinon problem.

Installing @types/sinon v1.16.36 seems to yield the best results, as it is closest available version to the UI5's 1.14.1

npm i @types/sinon@1.16.36 --save-dev