SAP / ui5-typescript

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

RouteInfo arguments type #419

Closed whydrae closed 1 year ago

whydrae commented 1 year ago

While using RouteInfo type in the TS app, I noticed that type of arguments is Record<string, string>. In reality, the second part is usually an object.

Following the documentation, we have the pattern defined as employees/{employeeId}/resume:?query: and then a tab parameter inside:

"?query": {
  tab: ...
}

Could you please take a look?

akudev commented 1 year ago

@whydrae Which place in the linked documentation side exactly do you refer to? I only see this object with tab: passed to the navTo(...) method of the router. But this parameter is not typed as "RouteInfo", so it doesn't matter in this place how RouteInfo is typed.

In the three libraries with routers (core, m, f), I only see RouteInfo being used in core, in the getRouteInfoByHash(...) method, where it is only a return type, not the type of a parameter. We can check there, whether it is correct, but anyway it wouldn't cause the trouble that I think you reported.

Can you elaborate a bit?

whydrae commented 1 year ago

@akudev, thanks for the prompt reaction! By referring to the documentation, I wanted to point out, that query parameters are usually not type of string, but rather an object.

In my code, I use the following controller function to retrieve the current route arguments:

getCurrentRoute(): RouteInfo {
    const router = this.getRouter(); // this.getOwnerComponent().getRouter();
    const currentHash = router.getHashChanger().getHash();
    return router.getRouteInfoByHash(currentHash); // returns typeof RouteInfo
}

Then, when I want to get the arguments themselves, I had to cast to unknown first, otherwise I get an error:

type MyQueryRouteArgumets = {
  "?query": {
    tab: string;
  };
};

...

const currentRoute = this.getCurrentRoute();
const args = currentRoute.arguments as unknown as MyQueryRouteArgumets;

Do you think this approach is correct, or should we rather have something like Record<string, string | object> for the arguments of RouteInfo?

akudev commented 1 year ago

Ok, that's the "we can check there" part - and apparently it is not correct because you can get something else back. However, would Record<string, string | object> cut it?

I don't think so: https://www.typescriptlang.org/play?#code/C4TwDgpgBASg9gV2BAkgOwGZygXlhAYzgCcATAHgGdhiBLNAcwBopq7GoAfKOAIwCtCwAHwBuAFDiMCNAWC04aKAwjB4SVJjgAKAJQAuWImTosUAN7io1qMVUJiS8wF8oAQ0pGNpuBOeSiNGpbY00zPBU1UJ89CXFA4IBbEABFBAhiEAAFN2I3RM88YmitAG0AIgB+AEd0zPKAXTiE4ChgN15cKGS0jOzc-MoAOnbeCSA

Even with Record<string, string | Record<string, string | string>> or actually Record<string, Record<string, unknown>> one first would have to check whether it is a string to be allowed to access it as record. (https://www.typescriptlang.org/play?#code/C4TwDgpgBASg9gV2BAkgOwGZygXlhAYzgCcATAHgGdhiBLNAcwBopq7GoAffIsqm+s1YCO3NoIB8EgNwAoWRgRoCwWnDRQGEYPCSpMcABQBKAFyxEydFigBvWVEdRi2hMQ22AvlACGlC3rWcHKe8kRo1M6W+jZ4WjrRQSZysuGRALYgAIoIEMQgAAo+xD7p-njEiQYA2gBEAPwAjrn5tQC6KbQYUIagkHDdmTl5hcWl5Th4teKMtcZ2Dk5pwFDAPgBGuFBDLaMlZSFQEAA2lND2TlDLqxtbOyNF+5QAdGvrIUA)

akudev commented 1 year ago

Actually, the type was defined as response to https://github.com/SAP/openui5/issues/3359 and apparently did not consider all cases.

I've opened internal incident 2370112658 for this and the responsible colleagues will take over.

stopcoder commented 1 year ago

Hi @whydrae,

we extended the RouteInfo with this commit. It will be released from the UI5 version 1.120. Do you need the fix to be downported to the release branches? If yes, can you please tell me the version number?

Best regards, Jiawei

whydrae commented 1 year ago

@stopcoder, thanks for the modification! I develop with 1.119 currently, but usually update to the newer version. So there's no real need to downport, if it's additional overhead.