fix(ThemeFactory): generic types

[JackUait]

fixes IF-283

Из-за фичи с наследованием типов дженериков, в функцию ThemeFactory.create можно было передать любое значение и она бы подстроилась под него

Представим себе ситуацию: мы передаём в ThemeFactory.create значение, которое не существует в теме

Вот такой код для кастомных значений работал раньше:

// Из-за наследования типов дженериками мы можем не передавать `someValueThatDoesNotExistInTheme` в дженерик
// Следовательно, в данной реализации мы имеем автокомплит, но не ограничиваем пользователей в значениях,
// которые они могут передавать в `ThemeFactory.create`
ThemeFactory.create({
    someValueThatDoesNotExistInTheme: '20px'
}, SOME_THEME)

Вот такой код нужен, чтобы завести ThemeFactory.create с кастомными значениями сейчас:

// Теперь если не передать кастомное значение в дженерик - код выдаст ошибку
ThemeFactory.create<{someValueThatDoesNotExistInTheme: string}>({
    someValueThatDoesNotExistInTheme: '20px'
}, SOME_THEME)

[zhzz]

ThemeFactory.create` не ругается ...

Но если начнет, как, я полагаю, в задаче и подразумевается, то это будет ломающее изменение.

[JackUait]

Такой же код работает и сейчас, ThemeFactory.create не ругается, только тип кастомной переменной в итоге - unknown.

Да, всё верно, я привёл некорректный пример

Данный PR решает две проблемы: запрещает пользователям передавать какое-угодно значение в ThemeFactory.create (теперь пользователи могут передавать только объект), а также решает проблему, при которой если в дженерик передан тип, то функция не подсвечивает какие поля в объекте из первого аргумента не входят в тип

Это достигается двумя улучшениями:

1. Ограничением передаваемых в дженерик типов до Record<string, unknown>;
2. Добавлением типа NoInfer, который отключает автоматическое наследование типов, другими словами когда мы передаём в объект поле someValueThatDoesNotExistInGenericType: 'abc' - дженерик не создаёт под него тип someValueThatDoesNotExistInGenericType: string, а говорит, что такого поля нет в типе

Но я не учёл одну штуку: второе улучшение (с NoInfer) имеет смысл, только если T задать какое-то значение или значение по умолчанию (т.е. T extends AnyObject = {}). Иначе, если обращаться к несуществующим в новой теме переменным - компилятор не будет говорить, что таких переменных нет, он будет считать что они есть, но тип у них unknown, как ты подметил в своём комментарии

Как можно сесть сразу на два стула (улучшить типизацию и не делать ломающих изменений): сейчас, вся соль текущего решения сводится к тому, что у нас T расширяет Record<string, unknown>, тем самым, создавая unknown типы для полей в объекте с пустым дженериком (что само по себе является ломающим изменением). То есть, нам достаточно заменить Record<string, unknown> на Record<string, string>, чтобы не ломать текущее поведение и улучшить типизацию на тот случай, если в дженерик ThemeFactory.create всё-таки передан тип

Что можно сделать в будущей, пятой версии библиотеки: добавить значение по умолчанию ({}) для дженерика, чтобы сделать поведение, при котором пользователи обязаны передавать объект для кастомных значений - поведением по умолчанию

[JackUait]

Сначала не заметил этого, но сейчас обнаружилась ещё одна проблема с текущим решением:

export const Theme2022DarkInternal = Object.setPrototypeOf(
  exposeGetters(Theme2022Dark),
  DefaultThemeInternal,
) as typeof Theme2022Dark; // Используется тип класса

Из-за того, что тип Theme2022DarkInternal приводится к типу класса, у нас получается расширять дженерик только от Record<string, any>. Типы Record<string, unknown> и Record<string, string> не считаются валидными типами, так как они не полностью покрывают типы, которые можно передать в класс

Тип Record<string, any> будет проблемой в том случае, если пользователь не передал ничего в дженерик:

const newTheme = ThemeFactory.create({ blahBlah: 'someValue' }, DEFAULT_THEME);
newTheme.blahBlah; // Тип newTheme.blahBlah - any

С другой стороны, можно сослаться на то, что пользователь можно расширять свою тему любым значением и мы не вправе ограничивать пользователя одними строками - в таком случае решение с Record<string, any> правильное со всех ракурсов

[zhzz]

Если мы улучшим типизацию в каком-то кейсе, это все равно значит, что мы его сломаем. Предлагаю старый метод пока не трогать.

Но правка полезная и нужно добавить ее в 5.0. И чтобы не забыть / не потерять, а также дать возможноть пользователям ее уже использовать, можно сделать рядом новый метод. Он может переиспользовать старый create, но иметь новую типизацию. В мажорном релизе потом поменяем.

Что-то типа такого:

  public static typeSafeCreate<T extends Record<string, unknown> = ThemeIn>(
    theme: ThemeIn & NoInfer<T>,
    baseTheme?: Theme,
  ): Readonly<Theme & NoInfer<T>> {
    return this.create(theme, baseTheme);
  }

По поводу типа пользовательских переменных, думаю, мы не должны его ограничивать только строками.

И еще, т.к. NoInfer штука довольна специфичная, лучше ее поподробнее задокументировать. Например, описать не только что она конкретно делает, но еще и зачем.