Skip to content

Typescript module server #936

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.

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

Draft
wants to merge 6 commits into
base: feature/federated
Choose a base branch
from

Conversation

ciaranschutte
Copy link
Contributor

@ciaranschutte ciaranschutte commented Apr 12, 2025

  • converts all files to TS
  • ensures type is correctly in imports
  • uses underscore to denote unused param, supported in TS as unused param eg. (_req, res)...
  • add getCaughtErrorMessage
    • type guards for caught thrown errors
  • moves ES client to module and define consistent type from "new api" folder (this type returns typings from api calls)
    • which also means ES functions require more strict typing overall eg. providing full set of object properties
  • ConfigObject is no longer a partial type
  • config becomes an extended object which complicates types, leaving most of this as any for now
  • config "prop drilling" has a lot of duplicated typing for now, refactoring config has been discussed

Further improvements:

  • location of types. Decide if they are colocated or a distinct "types" folder

Comment on lines +1 to +27
import { Client, type ClientOptions } from '@elastic/elasticsearch';

export const buildEsClient = (esHost = '', esUser = '', esPass = '') => {
if (!esHost) {
console.error('no elasticsearch host was provided');
}

const esConfig: ClientOptions = {
node: esHost,
};

if (esUser) {
if (!esPass) {
console.error('ES user was defined, but password was not');
}
esConfig['auth'] = {
username: esUser,
password: esPass,
};
}

return new Client(esConfig);
};

export const buildEsClientViaEnv = (ES_HOST: string, ES_USER: string, ES_PASS: string) => {
return buildEsClient(ES_HOST, ES_USER, ES_PASS);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move ES client to module

import { Client, type ClientOptions } from '@elastic/elasticsearch';

export const buildEsClient = (esHost = '', esUser = '', esPass = '') => {
if (!esHost) {
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it be fair to provide a default value for this?
ES has a common 9200 port, and localhost is a usual

Comment on lines +12 to +15
import type { Client } from '@elastic/elasticsearch';
import type { SQONFilter } from '#sqon/index.js';
import { getCaughtErrorMessage } from '#utils/caughtError.js';
import type { GraphQLSchema } from 'graphql';
Copy link
Member

Choose a reason for hiding this comment

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

please, would you add these in the order/position that keeps consistency with code already present?
I've found that using this Microsoft extension for ESLint helps a lot keeping track of this and other "consistency" items https://marketplace.visualstudio.com/items/?itemName=dbaeumer.vscode-eslint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handy extension thank you 👍 they become "out of sync" with VSCode "import this file" which we now have after ts-ify 👍

@@ -294,15 +307,26 @@ export const createSchemasFromConfigs = async ({
schema: fullSchema,
};
} catch (error) {
const message = error?.message || error;
const message = getCaughtErrorMessage(error);
Copy link
Member

Choose a reason for hiding this comment

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

👍 one more from my ToDo:s, thanks for handling it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants