Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ServerConfig.d.ts: add types for options #7328

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ServerConfig.d.ts: add types for options #7328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/Core/CorsProxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import defined from "terriajs-cesium/Source/Core/defined";
import { type ServerConfigOptions } from "./ServerConfig";
import URI from "urijs";

/**
Expand Down Expand Up @@ -62,7 +63,7 @@ export default class CorsProxy {
* @returns A promise that resolves when initialisation is complete.
*/
init(
serverConfig: any,
serverConfig: ServerConfigOptions | undefined,
baseProxyUrl: string = CorsProxy.DEFAULT_BASE_PROXY_PATH,
proxyDomains: string[] = []
) {
Expand Down
16 changes: 14 additions & 2 deletions lib/Core/ServerConfig.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
export declare interface ServerConfigOptions {
version: string;
proxyAllDomains: boolean;
allowProxyFor: string[];
maxConversionSize: number;
newShareUrlPrefix?: string;
shareUrlPrefixes: object;
additionalFeedbackParameters: object[];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


declare class ServerConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

config: unknown;
init(serverConfigUrl: string): Promise<unknown>;
config: ServerConfigOptions;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way server endpoint will be called only once, or do you mean about the server config stored in the terria class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

init(
serverConfigUrl: string | undefined
): Promise<ServerConfigOptions | undefined>;
}

export default ServerConfig;
2 changes: 1 addition & 1 deletion lib/Core/ServerConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ function ServerConfig() {
* Contains configuration information retrieved from the server. The attributes are defined by TerriaJS-Server but include:
* version: current version of server
* proxyAllDomains: whether all domains can be proxied
* allowProxyFrom: array of domains that can be proxied
* allowProxyFor: array of domains that can be proxied
* maxConversionSize: maximum size, in bytes, of files that can be uploaded to conversion service
* newShareUrlPrefix: if defined, the share URL service is active
* shareUrlPrefixes: object defining share URL prefixes that can be resolved
Expand Down
9 changes: 6 additions & 3 deletions lib/Models/Terria.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import {
} from "../Core/Json";
import { isLatLonHeight } from "../Core/LatLonHeight";
import Result from "../Core/Result";
import ServerConfig from "../Core/ServerConfig";
import ServerConfig, { type ServerConfigOptions } from "../Core/ServerConfig";
import TerriaError, {
TerriaErrorOverrides,
TerriaErrorSeverity
Expand Down Expand Up @@ -650,7 +650,7 @@ export default class Terria {
this.forceLoadInitSources.bind(this)
);

@observable serverConfig: any; // TODO
@observable serverConfig?: ServerConfig; // TODO
@observable shareDataService: ShareDataService | undefined;

/* Splitter controls */
Expand Down Expand Up @@ -2129,7 +2129,10 @@ export default class Terria {
});
}

async initCorsProxy(config: ConfigParameters, serverConfig: any) {
async initCorsProxy(
config: ConfigParameters,
serverConfig: ServerConfigOptions | undefined
) {
if (config.proxyableDomainsUrl) {
console.warn(i18next.t("models.terria.proxyableDomainsDeprecation"));
}
Expand Down
1 change: 1 addition & 0 deletions lib/Models/sendFeedback.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export default function sendFeedback(options: {
};
if (
options.additionalParameters &&
terria.serverConfig &&
terria.serverConfig.config &&
terria.serverConfig.config.additionalFeedbackParameters
) {
Expand Down