ServerConfig.d.ts: add types for options #7328
Conversation
| declare class ServerConfig { | ||
| config: unknown; | ||
| init(serverConfigUrl: string): Promise<unknown>; | ||
| config: ServerConfigOptions; |
There was a problem hiding this comment.
It's not for this PR, but I find the ServerConfig design a bit puzzling, the object caches a configuration, but ServerConfig.init() that uses the cached configuration is only called from Terria.start(), and that call is always done directly after new ServerConfig(). To me, it seems it would have been easier to have ServerConfig.init() be the equivalent of a static function in other languages so ServerConfig object would never have to be allocated at all.
There was a problem hiding this comment.
This way server endpoint will be called only once, or do you mean about the server config stored in the terria class?
There was a problem hiding this comment.
My point is that terria.start() looks like this:
this.serverConfig = new ServerConfig();
const serverConfig = this.serverConfig.init(...);so init is only called immediately after new, and the cache will never be used. The ServerConfig object could just as well have been omitted and the code looked like this instead:
this.serverConfigOptions = mkServerConfigOptions(...);which would omit one level of indirection (and save a few bytes of object allocation), and otherwise have provided the same performance characteristics.
| newShareUrlPrefix?: string; | ||
| shareUrlPrefixes: object; | ||
| additionalFeedbackParameters: object[]; | ||
| } |
There was a problem hiding this comment.
p.s. It might be better to define this interface in terriajs-server, but from the top of my head, I am not sure if it is possible.
There was a problem hiding this comment.
I have PRs on both terriajs-server and TerriaMap from early May that are still sitting in the queue either approved or without any comments (and some of them are ~one-liners), so I feel that even if you are right on a technical level, it's unlikely to be productive for me to try to migrate existing interfaces between repositories.
| shareUrlPrefixes: object; | ||
| additionalFeedbackParameters: object[]; | ||
| } | ||
|
|
||
| declare class ServerConfig { |
There was a problem hiding this comment.
What do you think about converting the entire ServerConfig class to typescript? it is relatively small and it would be easier then to have an additional declaration file
There was a problem hiding this comment.
Well, I know type systems so I'm happier the more types I get, but #6466 has been sitting for several years by now. I would rather get this PR merged in its current form so we get some improvements than have another "perfect" PR that sits around for 6+ months waiting for someone to look at it.
There was a problem hiding this comment.
@zoran995 re-reading my comment made me realize that it could be interpreted as criticism against you, and that was absolutely not what I meant, so I apologize for that. I'm grateful for all your help, there's not a chance in a million years I would have found the issue with createRef/useRef that you brought to my attention in #7213.
I'm still running with my training wheels with TypeScript/JavaScript, so I TSified something that already had complete type annotations in #7330 instead.
There was a problem hiding this comment.
No worries at all - I didn't interpret it as a criticism, don't worry. I also "hate" that PR for staying open for so long and being that big.
Thanks for all your work, you are doing an amazing job splitting everything into small PRs
| declare class ServerConfig { | ||
| config: unknown; | ||
| init(serverConfigUrl: string): Promise<unknown>; | ||
| config: ServerConfigOptions; |
There was a problem hiding this comment.
This way server endpoint will be called only once, or do you mean about the server config stored in the terria class?
pjonsson
force-pushed
the
type-serverconfig-more
branch
from
0623159 to
937d4db
Compare
Add some types for the options in ServerConfig. This uncovered a missing check in sendFeedback.
pjonsson
force-pushed
the
type-serverconfig-more
branch
from
937d4db to
35f9a35
Compare
What this PR does
Add some types for the options in
ServerConfig. This uncovered a missing
check in sendFeedback.
Test me
Tested by CI.
Checklist
doc/.