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

Can´t get access to generated Event Parameter in code completion #404

Closed marianfoo closed 1 year ago

marianfoo commented 1 year ago

Describe the bug Trying to use the parameters from the event like described in the changelog here for version 1.115.0 and 1.115.1.
But no parameters are displayed as defined.

Expected behavior Expect to see a suggestion as here image

No suggestion image

Additional context

Recreate like

git clone https://github.com/marianfoo/ui5-cc-spreadsheetimporter
git checkout chore/update-ts-sample
pnpm i
pnpm build

Generated Component types here: https://github.com/marianfoo/ui5-cc-spreadsheetimporter/blob/e661c3ea509c6a8cc5631a24587ea7901eb504a3/packages/ui5-cc-spreadsheetimporter/src/Component.gen.d.ts

Trying to use those event parameters here: https://github.com/marianfoo/ui5-cc-spreadsheetimporter/blob/chore/update-ts-sample/examples/packages/ordersv4fets/webapp/ext/ObjectPageExtController.ts

Anything is missed in the tsconfig or at generation?

Also, after an update of the ts-interface-generator this line was not there anymore. Is this neccesary? see https://github.com/marianfoo/ui5-cc-spreadsheetimporter/blob/e661c3ea509c6a8cc5631a24587ea7901eb504a3/packages/ui5-cc-spreadsheetimporter/src/Component.gen.d.ts#L623C1-L630C2

// This module enhances sap.ui.base.Event with Generics, which is needed in UI5 type definition versions below 1.115
declare module "sap/ui/base/Event" {
    export default interface Event<ParamsType extends Record<string, any> = object> {
        constructor(id: string, oSource: EventProvider, parameters: ParamsType);
        getParameters(): ParamsType;
        getParameter<ParamName extends keyof ParamsType>(name: ParamName): ParamsType[ParamName];
    }
}
akudev commented 1 year ago

At first glance, the generated interface is complete (the event-related things, the parameters type and the event itself are there).

The part starting with "This module enhances sap.ui.base.Event with Generic" is not needed anymore once you as control developer are working with the 1.115 types, which you are. This is detected automatically. The consumer of the control should have at least the same version then - we cannot know this at the time the interface is generated, of course.

However, that event has type "any", even though the event type is explicitly assigned (this should not even be necessary when attaching the event handler programmatically), is strange. This looks like Component$CheckBeforeReadEvent isn't really available at this location. But I also don't see any error marker for it.

I'll clone and have a deeper look later. Maybe something related to Component vs Control (we focus on the latter).

marianfoo commented 1 year ago

Thanks @akudev The consuming application is at 1.108.19 and i´m using 1.115.1. As i am using a custom component, that shouldnt be a problem, right?

Even when i use explicitly i get any for typedEvent image

akudev commented 1 year ago

Well, that could be exactly the problem! Please try re-inserting this block anywhere (maybe not in the generated interfaces because it will be removed again the next time it is generated):

// This module enhances sap.ui.base.Event with Generics, which is needed in UI5 type definition versions below 1.115
declare module "sap/ui/base/Event" {
    export default interface Event<ParamsType extends Record<string, any> = object> {
        constructor(id: string, oSource: EventProvider, parameters: ParamsType);
        getParameters(): ParamsType;
        getParameter<ParamName extends keyof ParamsType>(name: ParamName): ParamsType[ParamName];
    }
}

The definition of the event is using Event as class with generics, which it is not in 1.108. For such broken definitions, TypeScript makes the type any.

I didn't want to generate this block in all versions, but maybe I should for such usage in lower versions. Or downport the Event change in OpenUI5, so at least later patches of the types would have it.

akudev commented 1 year ago

That's the Caveat I described in the commit message:

ONE CAVEAT:
The generated types make use of the fact that sap.ui.base.Event is a
class with generics since version 1.115 of the UI5 types. For use with
lower versions of the UI5 types, the generator can create a kind of shim
that makes the Event class generic. This happens automatically based on
the version of the UI5 types AT CONTROL DEVELOPMENT TIME (i.e. only when
the control developer uses a version of the UI5 types lower than 1.115).

If, however, the application developer USING the control uses a
different version of the UI5 types than the control developer, then the
following happens:
1.) App uses >=1.115, Control uses < 1.115: the shim is there but does
not hurt.
2.) App uses <1.115, Control uses >= 1.115: the shim is missing and the
generated *.gen.d.ts file is not valid in the context of the app's UI5
types. However, TypeScript simply makes the type "any", so there there
is no big problem on application side, only reduced type safety.
Furthermore, "skipLibCheck" needs to be set to "true", otherwise the
TypeScript compiler reports an error when checking the UI5 types and the
generated interface files.
marianfoo commented 1 year ago

It does look better now, but i still don´t get the code completion for the parameters image

image

But I basically underestimated the problem of providing Typescript for different versions. Basically the component is usable for all versions from 1.71 and up. Can you recommend a strategy here? Staying on 1.108 or can i always use the newest?

akudev commented 1 year ago

Even without explicitly assigning the type (and then even casting again like you did), I do get the code completion after using the 1.115.1 types in the app. This is for the result of getParameters():

image

And I do think you also get them in the upper screenshot as well - sheetData is offered there for completion.

I also get the completion for the string parameter in getParameter(): image

But in VSCode it is indeed a bit tricky to get the string suggestions. It works for me when I first type event.getParameter() without parameter and then type quotes inside the brackets. It also works when I type event.getParameter(""), then place the cursor between the quotes and ask for suggestions. It does, however, not work when I write event.getParameter("" and then ask for code completion between the quotes. Try around a little, it's not a problem of the types.

marianfoo commented 1 year ago

Ok, did you change anything beside adding declare module "sap/ui/base/Event"? Otherwise it seems this is a error on my client side i need to figure out, because they don´t even appear when i use getParameters.
I would then close the issue as it is not related to your code base.

marianfoo commented 1 year ago

Sorry, as you said you updated the @sapui5/ts-types-esm to 1.115.1. After i did that it works. It's not that easy to know which features are available and which are not, depending on which version you are working with. Currently you have to go through the changelogs? Will there be an easier guide in the future?

akudev commented 1 year ago

Argh, yes, I basically did the wrong thing, as the main question was about 1.108. Going back to the 1.108.0 types in the app, what one can observe is:

  1. adding the Event module re-definition from above inside Component.gen.d.ts does NOT make code completion work.
  2. adding it inside ObjectPageExtController.ts does NOT work either.
  3. adding it inside a new types.ts file inside the application does NOT work either.
  4. adding it inside a new types.d.ts file inside the application DOES work...
  5. ...but as soon as one writes something like import EventProvider from "sap/ui/baseEventProvider" at the top, it does NOT work again.

Why is that? It surely feels confusing (and this answer is not a result of me knowing immediately, but trying for a while), but I think is the result from a) having this declaration in the actual TypeScript context or not and b) having it in an ambient declaration or a module context. Especially 4. vs. 5. shows the latter, as this is exactly what the adding of an import statement causes.

I might have to revisit the generation of the .gen.d.ts files, as the imports make them non-ambient, but it's indeed all a bit confusing, as it works fine in my test scenario.

I also forgot to reply to "the problem of providing Typescript for different versions". Let me sort and develop my thoughts in a lengthy way...

Well. There's not only the TypeScript version, but the UI5 version, the UI5 types version, the version of the generator producing the UI5 types (the result can be quite different for the same UI5 version when using different versions of the generator), and then the versions of various tools around, like the ts-interface-generator. We won't be able to make statements for all possible combinations of them what works and what doesn't for all possible things that users of the type definitions could do (from app development up to providing re-use components where the component and the re-using context might have different combinations of all the above versions.

But I think in real life it's a bit easier:

So the problem we need to take care of is narrowed down to the TypeScript code generated ts-interface-generator generating which is working well a) with older TypeScript versions and b) with older UI5 types.
a) is already happening (albeit not well covered by automated tests), b) is something that has only popped up recently with the extended Event definition. This indicates at least that such issues are not happening all the time, but only in exceptional cases. And it teaches us what to look out for. Actually we did look out for it and generate this "compatibility block" when the developer has older UI5 types, but of course this doesn't solve it for users with older types.

All in all, however, actually I would argue that nothing is broken: If the user of your re-use library/component has the UI5 types 1.115.1 or higher, then the typed events are available, otherwise the typed events are not available, but everything still works. Actually it works consistently with the events from standard UI5 controls, so that developer wouldn't even notice that something is missing.

Regarding the "easier guide than the changelog", we'll have to figure out. Apparently we kind of notice such changes when rolling them out, so we could list them separately. But yeah, I think we have to collect some experience.
And as I wrote above, I don't think that you need to inform your users that your re-use stuff only works with certain type versions. It does work with lower versions, as good as the standard UI5 controls work.

In general, though, the generated TypeScript code can have dependencies on a certain version of TypeScript and the UI5 types which are more breaking than this one. So testing is required and specifying which versions you used.

As result of thinking about this, we'll likely start generating the UI5 types version and TypeScript version numbers into the generated files to make issues easier to analyze.

marianfoo commented 1 year ago

Wow, thanks for the work and the detailed presentation.

You are right, if you write only one application, it is relatively easy. But what stayed in my head is your statement at the last UI5erslive best to use the UI5 version with the analog UI5 Typescript version. So I tried to implement this accordingly in the application. But since I write the component for several versions, I just use the latest UI5 Typescript version. Right, here I am more dependent on the generator and the transpiling task of Peter.

Since I provide the Typescript types, I'm afraid that the developers of my component will come to me and ask why they don't work. A statement which UI5 version works with which UI5 Typescript version and generator is really a bit too much to ask. I would like to have a general statement like "Use the latest UI5 Typescript version" or "Use at least the UI5 Typescript version for the UI5 version". I know that I am not easy, but I would like to avoid obvious errors. Would be a good strategy then to test which LTM versions just work with the latest UI5 Typescript versions? Or are those errors above fixable by updating the generator? I am not sure yet how to proceed.

Is there actually a minimum UI5 version for Typescript?

akudev commented 1 year ago

best to use the UI5 version with the analog UI5 Typescript version

I think this is a good general rule.

And still one can use newer UI5 types with older UI5 runtime (e.g. to get better type support, like typed events) or a newer runtime with older types (e.g. when the type definitions get a breaking change, but one wants/need to update the UI5 runtime version). But o course you are quickly in "you need to know what you are doing" land when you do so.

since I write the component for several versions, I just use the latest UI5 Typescript [=UI5 types] version

Reasonable.

I'm afraid that the developers of my component will come to me and ask why they don't work

In general: yes. This is why I would say you need to test the usage with lower versions of the UI5 types and make a clear statement which versions are supported. Actually I don't think this is different from e.g. writing an npm package and testing with lower NodeJS versions. Then you declare the minimum required Node version. Or when writing a UI5 app: at least when it is supposed to be deployed into different FLPs, the manifest needs to declare the minimum required UI5 version. This may be way lower than the version you used while developing. But you still need to test it. Or check in the documentation of each an every used API since when it exists (and hope nothing else changed).

In this specific case: no. No developer using the 1.108 types will come to you and ask why your events are not fully typed. All the standard UI5 control events are not typed either in 1.108.

test which LTM versions just work with the latest UI5 Typescript versions

Define "just work".

As long as one simply talks about plain application development, the answer is something like "all and none", at the same time:

When we are now talking about developing some re-use stuff, then the answer is similar and again depending on what exactly does "work" mean:

This issue report clearly makes us aware of this question and you can expect some sensitivity for this topic even though we don't have an immediate answer on everything yet, like how such breaks are defined (you know... what does "work" mean?) and announced.

are those errors above fixable by updating the generator?

They might be. As they are caused by the generated TypeScript code, I would generally expect that they can also be fixed by generating different code. In practice we have already seen something like this here: these 163 lines of tedious AST construction generate those 6-7 lines of compatibility code, which makes the events in your control typed even when all other controls of UI5 have no typed events.

For instances of something really breaking I would estimate this could also be prevented, but I can't rule out that it might not be possible.

Is there actually a minimum UI5 version for Typescript?

The first UI5 version where we also published the new ESM types was 1.90. But this only says since when there are simultaneous types published. I'm pretty sure you can also develop against UI5 1.89 using the 1.90 types at compile time.

But yeah, I get it that it is hard to grasp in which cases exactly (and between which versions) the "you can use an older UI5 version" rule of thumb does not work. For now I would say let's settle on "there is an awareness on this topic" and get some comfort from the fact that nothing seems to have broken across many releases so far to my knowledge.

marianfoo commented 1 year ago

"you need to know what you are doing" land

The more I got involved with Typescript the more I think I'm still a quiet far away from it. Therefore, all the more, thank you for your support.

In this specific case: no. No developer using the 1.108 types will come to you and ask why your events are not fully typed. All the standard UI5 control events are not typed either in 1.108.

The more I understand now the more I agree with the statement

The first UI5 version where we also published the new ESM types was 1.90. But this only says since when there are simultaneous types published. I'm pretty sure you can also develop against UI5 1.89 using the 1.90 types at compile time.

As i´m supporting only LTM Versions, i would start to "support" for UI5/Typescript Version 1.96 and 1.108. Sounds reasonable to me.

Define "just work".

I think the just was unnecessary.
What I meant more is that the combination of UI5 and Typescript version works in the use cases that concern my component, such as the event parameters, but also other things. This means that I have to program the use cases for the current LTM version 1.96 and 1.108 and test if it works as intended (this was the original reason for the issue).
In this example here, I honestly didn't try the standard controls at all and thought that only affected my custom events.

As they are caused by the generated TypeScript code, I would generally expect that they can also be fixed by generating different code.

I think that is also a point that is difficult for me to see where the error is actually coming from now. I think the best course of action is to simply open a ticket, although I always like to try to solve the problem myself, it is always difficult to come up with a solution myself.

For now I would say let's settle on "there is an awareness on this topic" and get some comfort from the fact that nothing seems to have broken across many releases so far to my knowledge.

My comfort is that the UI5 developers with Typescript are still in the minority, but I think with leaving the experiment status that interest will hopefully also increase. I want to be ready for future problems :)

marianfoo commented 1 year ago

Thank you for the support!
I think it is a little bit clearer now.