SAP / openui5

OpenUI5 lets you build enterprise-ready web applications, responsive to all devices, running on almost any browser of your choice.
http://openui5.org
Apache License 2.0
2.95k stars 1.24k forks source link

"Unresolved variable or type…" using `@types/openui5` #3403

Closed pubmikeb closed 2 weeks ago

pubmikeb commented 2 years ago

OpenUI5 version: 1.97.0

In IDEA 2021.3, I've added @types/openui5 of 1.97.0 to the list of the JavaScript libraries. I've checked the content of sap.ui.core.d.ts, it does contain the definition of UIComponent. But at the same time, IDEA is not capable of resolving the sap.ui.core.UIComponent:

29_223817

I've opened a ticket at IDEA: https://youtrack.jetbrains.com/issue/WEB-52649, but would like to understand is it an IDEA issue or the @types/openui5 definitions one?

georgimkv commented 2 years ago

Hello @pubmikeb Thank you for sharing this finding. I've created an internal incident 2180394945. The status of the issue will be updated here in GitHub. Regards, Georgi

pubmikeb commented 2 years ago

Additional info including the sample project: https://github.com/SAP/openui5/pull/3371#issuecomment-954238404.

akudev commented 2 years ago

Hi @pubmikeb Have you had a look at https://github.com/SAP-samples/ui5-cap-event-app/blob/js-with-typescript-support/README.md? In particular this section: https://github.com/SAP-samples/ui5-cap-event-app/blob/js-with-typescript-support/README.md#provide-type-information-for-loaded-dependencies

The @types/openui5 definitions do no longer support using global variables like sap.*. This is to promote using modern JavaScript language features like ES modules in newly written TypeScript apps. For JavaScript coding supported by those type definitions this unfortunately means that the type references in JSDoc need to be written in a module import way as well, as explained in the last link above:

/**
 * @param {typeof import('sap/ui/core/mvc/Controller').default} Controller
 * @param {typeof import('sap/m/MessageBox').default} MessageBox
 * @param {typeof import('sap/m/MessageToast').default} MessageToast
 * @param {typeof import('sap/m/InputBase').default} InputBase
 * @param {typeof import('sap/ui/model/Filter').default} Filter
 * @param {typeof import('sap/ui/model/FilterOperator').default} FilterOperator
 */
function (Controller, MessageBox, MessageToast, InputBase, Filter, FilterOperator) {

Please try typing the function parameters in this way!

If, e.g. for existing applications, you still need to use the global names, you can try using the @openui5/ts-types package of type definitions. This flavor of the UI5 type definitions still supports the globals and should be equivalent to (actually better than) the prior @types/openui5 pakages. However, it's not the flavor which is in our focus for the future, so switching over to the module way would be a good thing in new projects.

Regards Andreas

pubmikeb commented 2 years ago

@akudev, thanks a lot for the reference!

BTW, is there any difference between:

@param {import("sap.m.library")} mLibrary - …

and

@param {typeof import("sap/m/library").default} mLibrary - …

in terms of pure JavaScript project and @types/openui5.

It looks like both work OK, but perhaps there are still some difference?

pubmikeb commented 2 years ago

One more thing regarding the types, even after providing the type references in a new way:

sap.ui.define([ // eslint-disable-line no-undef
        "sap/ui/core/library",
        "sap/ui/core/UIComponent"
    ],

    /**
     * @param {typeof import("sap.ui.core.library").default} uiCoreLibrary
     * @param {typeof import("sap.ui.core.UIComponent").default} UIComponent
     * @returns {Function}
     */
    (uiCoreLibrary, UIComponent) => UIComponent.extend("com.mySandboxApp", {

        metadata: {
            interfaces: [
                uiCoreLibrary.IAsyncContentCreation
            ],
            manifest: "json"
        }

    }));

I still get complaints regarding IAsyncContentCreation being unresolved, although uiCoreLibrary has a proper JSDoc signature:

@param {typeof import("sap.ui.core.library").default}

Did I forget something?

akudev commented 2 years ago

@pubmikeb Regarding the difference in notations, fwiw in my VSCode it makes a big difference. I did all permutations of omitting "typeof" and ".default" with different error messages as result (the last one a bit irritating because it references a "typeof" which is not in the code):

image

image

image

Regarding IAsyncContentCreation,... all the other references are apparently working? That's good at least. But the core library... I actually cannot import its default export in JSDoc in VSCode: image

And that's correct, because in the type definitions the core library does not have a default export. It only has named exports, including this one:

  export interface IAsyncContentCreation {
    __implements__sap_ui_core_IAsyncContentCreation: boolean;
  }

And actually, VSCode does offer me this named import as I start typing its name: image

So when I write

@param {import('sap/ui/core/library').IAsyncContentCreation} IAsyncContentCreation

it works properly.
Note, however, that this is now without typeof! With typeof, there is an error: image

Ok, I see, there is some work ahead understanding and explaining in detail how the JSDoc type annotations should be done. For the time being I'd recommend experimenting along the above ideas when something doesn't work.

In general, also in proper TypeScript, libraries usually use named exports for their members, controls only export the control itself as default export.

I also see some apparent differences in how different IDEs handle JSDoc comment when TypeScript type definitions are available. From what I have read and seen above, VSCode seems to work a bit better than IDEA?

pubmikeb commented 2 years ago

@akudev, thanks for your comparisons.

I did all permutations of omitting "typeof" and ".default"

actually, I meant / versus . in the path:

"sap.m.library"

vs.

"sap/m/library"

Is there any reason why should I prefer / over . in UI5 JSDocs?

From what I have read and seen above, VSCode seems to work a bit better than IDEA?

I would say, it's different, in some cases IDEA has a flexiber JSDoc parser, at least

@param {import("sap.m.library")} mLibrary

works perfectly in IDEA and mLibrary.URLHelper.redirect(…) is correctly resolved, while according to your experience, without typeof / .default VSCode returns some errors.

akudev commented 2 years ago

Oh! :-) Missed that detail. But still it doesn't work: image (and when setting module resolution to "Node" still no success) image

This is expected because actually the type definitions do not define anything named "sap.ui.core.mvc...". Probably your observation is right that IDEA interprets JSDoc in a more relaxed/tolerant way.

Actually, when writing JavaScript, one could also use the @...ui5/ts-types definitions which still define the global objects, so the nicer way of specifying the types can be used. But we decided against recommending this because we don't intend to support those legacy definition files equally well.
It could be an option to additionally generate definition files which just re-add the globals and point the defined modules. These definitions could be loaded in addition to combine supported definitions and straightforward notation. But we won't have capacity for this anytime soon.

All in all, is there something left from this issue, other than exploring the JSDoc parsing in different IDEs?

codeworrior commented 2 years ago

It could be an option to additionally generate definition files which just re-add the globals and point the defined modules. These definitions could be loaded in addition to combine supported definitions and straightforward notation. But we won't have capacity for this anytime soon.

Ehm, do you remember? We tried that in the beginning, and it didn't work out. Initially, this was our preferred approach over having two separate type worlds which can't be mixed. If I remember it correctly, one of the problems was that the necessary imports are not allowed in ambient files (only in modules) and another one was the dual type-and-value nature of e.g. enums that we didn't manage to re-export in namespaces. IIRC (2), there had been many discussion around similar topics in the web community and they all ended up in going either way (globals or modules).

Well, but if someone has a good proposal how to achieve this, I would be glad to give it a second try.

pubmikeb commented 2 years ago

All in all, is there something left from this issue, other than exploring the JSDoc parsing in different IDEs?

Regarding IAsyncContentCreation, I've tried:

@param {import("sap/ui/core/library").IAsyncContentCreation} IAsyncContentCreation

The full code snippet:

sap.ui.define([
        "sap/ui/core/library",
        "sap/ui/core/UIComponent"
    ],

    /**
     * @param {import("sap/ui/core/library").IAsyncContentCreation} IAsyncContentCreation
     * @param {import("sap/ui/core/UIComponent")} UIComponent
     * @returns {Function}
     */
    (IAsyncContentCreation, UIComponent) => UIComponent.extend("com.MySandboxApp", {

        metadata: {
            interfaces: [
                IAsyncContentCreation
            ],
            manifest: "json"
        }

    }));

But this prevents a view loading:

ModuleError: Failed to execute com.MySandbox/controller/Login.controller.js: Refused to evaluate a string as JavaScript because unsafe-eval is not an allowed source of script in the following Content Security Policy directive: "script-src https://*.hana.ondemand.com 'self'".

While the original variant works correctly and no warning occurs:

sap.ui.define([
        "sap/ui/core/library",
        "sap/ui/core/UIComponent"
    ],

    /**
     * @param {import("sap/ui/core/library")} uiCoreLibrary
     * @param {import("sap/ui/core/UIComponent")} UIComponent
     * @returns {Function}
     */
        // eslint-disable-next-line max-params
    (uiCoreLibrary, UIComponent) => UIComponent.extend("com.MySandboxApp", {

        metadata: {
            interfaces: [
                uiCoreLibrary.IAsyncContentCreation
            ],
            manifest: "json"
        }

    }));

Therefore, perhaps {import("sap/ui/core/library").IAsyncContentCreation} solves the JSDoc warning but then it raises another issue with metadata interface definition.

pubmikeb commented 1 year ago

Is there any update on the subject?

akudev commented 1 year ago

@pubmikeb Sorry for not responding. Well, yes, my code snippet was misleading. Importing sap/ui/core/library but claiming the @param is import("sap/ui/core/library").IAsyncContentCreation and using it as such will satisfy TS, but of course not work at runtime. It's still the library itself that is passed, because that's what sap.ui.define is told to do.

The easiest workaround for usage in JavaScript is probably to import the library itself, NOT type it, and then access core_lib.IAsyncContentCreation and type it as import("sap/ui/core/library").IAsyncContentCreation:

sap.ui.define([
    "sap/ui/core/library",
    "sap/ui/core/UIComponent"
],

/**
 * @param {any} core_lib
 * @param {typeof import("sap/ui/core/UIComponent").default} UIComponent
 * @returns {Function}
 */
(core_lib, UIComponent) => {
    const /** @type {import("sap/ui/core/library").IAsyncContentCreation} */ IAsyncContentCreation =  core_lib.IAsyncContentCreation;

    return UIComponent.extend("com.MySandboxApp", {
        metadata: {
            interfaces: [
                IAsyncContentCreation
            ],
            manifest: "json"
        }
    }
)});

A cleaner solution would be to type the Core library using a simple @typedef first because then everything works as expected (only the relevant part pasted below):

/**
 * @typedef {{IAsyncContentCreation: import("sap/ui/core/library").IAsyncContentCreation}} CoreLib
 */

/**
 * @param {CoreLib} core_lib
 * @param {typeof import("sap/ui/core/UIComponent").default} UIComponent
 * @returns {Function}
 */
(core_lib, UIComponent) => {
    const IAsyncContentCreation =  core_lib.IAsyncContentCreation;

You could now wonder why this is not the case out of the box, but, well, this is not the case right now. And it would mainly be a goody for JS development, while TS development (or rather anything using module imports) works well with the named imports. It's something that could be discussed, though, as it represents the runtime reality.