feat(ts-gen)!: replace legacy endpoints with new ones

[MartinBelthle]

ANT-2675

[hdinia]

question: their is no max in specs ? what if a user submit with a very large number

[MartinBelthle]

There's not really a max number. It's just an int that should be superior or equal to 1

[hdinia]

ok thanks I was wondering if a very large number for the generation could cause issues

[hdinia]

put in types file under service/.../.../timeseries (it's also related to api service)

[skamril]

No it's specific to the form, 'stochasticTsStatus' doesn't exist in the API anymore.

[hdinia]

Few remarks about the types and constants

• the API contract and Form UI state are mixed
  cf: type TSConfigValues = Record<TTSType, TSTypeConfig & { stochasticTsStatus: boolean }>;
  -> split to one for the Form state + one for the API contract

• some types are overly complex (usage of generics seems not appropriate here), same for type utilities from TS toolbelt cf:

export interface SetTimeSeriesConfigParams<T> {
  studyId: StudyMetadata["id"];
  // Extra fields not allowed by the API
  values: DeepPartial<F.Exact<T, TSConfigDTO>>;
}

-> please consider simplifying

• the naming especially the usage of the "T" prefix + abbreviations reduce clarity cf: TTSType

recommandations:

• remove abbreviations and prefix (you can define a type and an enum-like with the same declaration name)
• split concerns

import { TimeSeriesType } from "./constants";

interface TimeSeriesConfig {
  number: number;
}

export interface TimeSeriesFormData extends TimeSeriesConfig {
  isStochastic: boolean;
}

export type TimeSeriesType = (typeof TimeSeriesType)[keyof typeof TimeSeriesType];
export type TimeSeriesFormValues = Record<TimeSeriesType, TimeSeriesFormData>;
export type TimeSeriesDTO = Record<TimeSeriesType, TimeSeriesConfig>;

export interface UpdateGenerationConfigParams {
  studyId: string;
  data: TimeSeriesDTO;
}

[hdinia]

in contants.ts

export const TimeSeriesType = {
  THERMAL: "thermal",
} as const;

[skamril]

the API contract and Form UI state are mixed
cf: type TSConfigValues = Record<TTSType, TSTypeConfig & { stochasticTsStatus: boolean }>;
-> split to one for the Form state + one for the API contract

What's the problem to mix them? I already split them: TTSType and TSTypeConfig are in the "api" folder, and TSConfigValues in the form folder because it's specific to it (I answered already above).

some types are overly complex (usage of generics seems not appropriate here), same for type utilities from TS toolbelt cf:

It's not 'complex' for no reason (I wrote a comment), the PUT endpoint throw an error when we add extra fields in the config, but without the use of F.Exact, TypeScript doesn't complain about that. In my form I have extra fields, and this 'complexity' allow to show the error directly in the IDE, I know that I have to omit them before.

the naming especially the usage of the "T" prefix + abbreviations reduce clarity cf: TTSType

We already talk about that, it's a known convention to have the 'T' prefix in certain case.
You can see in you example that you have two TimeSeriesType: an enum-like and a type, it's confusing.
The type is based on the enum-like, so I kept the name of it, with a 'T' prefix, to prevent duplicate name.

For the abbreviations, like I said, I keep the same names as the backend types in the "api" folder, to simplify the search inside the code and the maintainability. But if @MartinBelthle can change the backend name, I will change them also.

[skamril]

import { TimeSeriesType } from "./constants";

interface TimeSeriesConfig {
  number: number;
}

export interface TimeSeriesFormData extends TimeSeriesConfig {
  isStochastic: boolean;
}

export type TimeSeriesType = (typeof TimeSeriesType)[keyof typeof TimeSeriesType];
export type TimeSeriesFormValues = Record<TimeSeriesType, TimeSeriesFormData>;
export type TimeSeriesDTO = Record<TimeSeriesType, TimeSeriesConfig>;

export interface UpdateGenerationConfigParams {
  studyId: string;
  data: TimeSeriesDTO;
}

• TimeSeriesFormData is not an API type.
• More simple to read O.UnionOf<typeof TimeSeriesType> rather than (typeof TimeSeriesType)[keyof typeof TimeSeriesType];. I don't know why you don't like it.
• TimeSeriesFormValues is not an API type
• TimeSeriesDTO is not precise, this the DTO of the config, this is why "Config" is added, without that we can think about matrices. The global config includes the specific config for each type, this is why we have: TSTypeConfig.

[hdinia]

setTimeSeriesConfig -> updateGenerationConfig

question: is generateTimeSeries responsible for creating the initial generation config, or something else ?

[skamril]

I prefer set over update, because it replaces all the config and not making partial modifications.
generateTimeSeries only read the config and create TS matrices.

[hdinia]

minor:

• improper use of uppercase constant notation (it's scoped to the Form)
• consider naming the accumulator when using a reduce, it make it easier to read. (opinion: the mutation of the acc not necessary here)

export const defaultValues = Object.values(TimeSeriesType).reduce(
  (values, type) => ({
    ...values,
    [type]: {
      number: 1,
      isStochastic: false,
    },
  }),
  {} as TimeSeriesFormValues,
);

[skamril]

improper use of uppercase constant notation (it's scoped to the Form)

Ok

consider naming the accumulator when using a reduce, it make it easier to read. (opinion: the mutation of the acc not necessary here)

We use acc almost everywhere (for me it's not a problem, because it is often small codes like here, but if you want more precise name you have to add a new regex in ESLint rule 'no-param-reassign').

Then, I prefer to use mutation with reduce every time, as it is safe with an initial value and, in all cases, more efficient than the spread operator. So why complicate things just for a code styling preference? This approach is simpler than mutating only when the object is large (what is the limit?...), especially since its size can sometimes change.

[hdinia]

minor:
the usage of the adapter pattern is good, but consider few improvements:

• this should live with api methods and be private to them (put in service file)
• the naming is too generic "toConfig" is broad

function formValuesToDTO(formValues: TimeSeriesFormValues): TimeSeriesDTO {
  return Object.entries(formValues).reduce((request, [type, values]) => {
    const { isStochastic, ...dto } = values;

    return {
      ...request,
      [type]: dto,
    };
  }, {} as TimeSeriesDTO);
}

best but optional:
you can use a facade function and expose methods for the service

// Private adapter
function formValuesToDTO(formValues: TimeSeriesFormValues): TimeSeriesDTO {
  return Object.entries(formValues).reduce((request, [type, values]) => {
    const { isStochastic, ...dto } = values;
    return {
      ...request,
      [type]: dto,
    };
  }, {} as TimeSeriesDTO);
}

export const timeSeriesService = {
  async createGenerationConfig(studyId: StudyMetadata["id"]) {
    const { data } = await client.put<string>(`/v1/studies/${studyId}/timeseries/generate`);
    return data;
  },

  async updateGenerationConfig(studyId: StudyMetadata["id"], formValues: TimeSeriesFormValues) {
    const data = formValuesToDTO(formValues);
    await client.put(`v1/studies/${studyId}/timeseries/config`, data);
  },
};

//usage
await timeSeriesService.createGenerationConfig(studyId);
await timeSeriesService.updateGenerationConfig(studyId, formValues);

[skamril]

this should live with api methods and be private to them (put in service file)

No, it's specific to the form, stochasticTsStatus is not in the API anymore.

the naming is too generic "toConfig" is broad

It's not toConfig but toConfigDTO. It is only include in the TS form folder (not a shared method), the function argument is typed, so it's clear in this context. It's not necessary to be too verbose here.

you can use a facade function and expose methods for the service

/v1/studies/{studyId}/timeseries/generate generate TS matrices, not a config.
Otherwise, I don't see the benefits of this encapsulation compared to the direct export of methods (in addition to maintaining consistency in the code).