feat: get rid of typings re-write main parts of the package in typescript #766

shadowusr · 2023-05-26T05:55:36Z

Что сделано?

Удалены все тайпинги .d.ts (кроме нескольких global)
То, что ранее входило в index.d.ts теперь переписано на typescript — значительная часть пакета переписана на TS
В результате у нас появляются всегда актуальные тайпинги, которые не нужно поддерживать отдельно
Пользователи получают сильную актуальную типизацию во всех контекстах работы с hermione

Принятые решения — можно обсудить

Полностью изменились экспорты из пакета (breaking change). Ранее экспортировались практически все типы и в формате namespace'а. Теперь экспортируется небольшой перечень классов и типов. Сам перечень экспортов и этот момент обсуждаем.
Код констант событий полностью переписан. Константы событий реализованы в виде объектов с const assertion'ами
Неудобная активация глобальных деклараций. Нужно подумать, как сделать использование глобальных деклараций конечными пользователями удобнее. Сейчас чтобы проросли типы it/describe/etc., нужно либо делать import "hermione";, либо прописать в tsconfig include: ["node_modules/hermione/build/src/index.d.ts"]. Оба решения так себе.

Как я тестировал?

Локально, unit-тесты. Нужно запустить на реальных проектах.

src/browser/commands/assert-view/errors/assert-view-error.ts

src/browser/commands/assert-view/errors/base-state-error.ts

src/browser/commands/assert-view/errors/image-diff-error.ts

src/runner/browser-runner.ts

src/runner/index.ts

src/test-reader/controllers/only-controller.ts

rprovodenko · 2023-06-08T10:13:11Z

Неудобная активация глобальных деклараций. <...>

Мне кажется, правильно сделать как в jest или mocha делается - отдельный npm пакет @types/hermione

https://www.typescriptlang.org/tsconfig#typeRoots

By default all visible ”@types” packages are included in your compilation.

А это значит что по дефолту у консьюмеров будут видны глобальные типы внутри @types/...

Нужно просто сделать ПР против DefinitelyTyped
https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#publish-to-types

shadowusr · 2023-06-08T23:22:49Z

Новые варианты касательно типов:

Можно заставлять пользователей писать такое в свой tsconfig.json:

{
    "compilerOptions": {
        "paths": {
            "*": ["node_modules/hermione"]
        }
    }
}

Создать @types пакетик
- Содержимое пакета — одна строчка /// <reference types="hermione" />
- Есть ограничение — этот @types пакет не может называться hermione, иначе он будет импортировать сам себя. Можно назвать что-то типа @types/hermione-globals. Но, тогда будут сложности на стороне DefinitelyTyped, ибо там пакеты должны соответствовать реальным, как я помню. Соответственно нужно будет скорее всего завести фейковый пакет
Создать реальный @types/hermione пакет без импортов типов из основного пакета
- Тут надо придумать какую-то генерилку этих тайпингов, ибо если их писать руками, то получим еще одно полотно тайпингов, которое надо поддерживать, что нивелирует профит от этого PRа. Как вариант — копировать все .d.ts файлы из основного пакета просто.
- Даже при авто-генерации его все равно нужно будет обновлять и выпускать руками

rprovodenko · 2023-06-09T06:51:01Z

Создать реальный @types/hermione пакет без импортов типов из основного пакета

Типы там ни откуда не надо импортировать. Мы генерим файлы тайпингов при билде гермионы автоматом (если это не настроено, легко настроить). Вот эти файлы c тайпингами мы и публикуем на DefinitelyTyped

Тут надо придумать какую-то генерилку этих тайпингов

Это делает тайпскрипт компилятор, через этот флаг https://www.typescriptlang.org/tsconfig/#declaration

Даже при авто-генерации его все равно нужно будет обновлять и выпускать руками

думаю уже есть паттерны на это (какое-нибудь автосоздание ПРа в DefinitelyTyped). Надо посмотреть

rprovodenko · 2023-06-09T07:03:16Z

Кстати, у jest есть еще вариант с импортами и другим пакетом @jest/globals

https://jestjs.io/docs/getting-started#type-definitions

rprovodenko · 2023-06-09T07:12:47Z

В общем, если мы хотим быть "как все", надо делать ПР в DefinitelyTyped. Здесь минус, что паблиш туда занимает какое-то время (до недели и больше), и это может стать затыком в релизе.

А можно просто забить и и делать импорты. Это легко и мы ни от кого не зависим.

А можно и то, и то сделать и дать юзеру выбрать как ему нравится (как jest сделали).

Мне нравится последний вариант, только без отдельного пакета по типу @jest/globals

DudaGod

🔥

DudaGod · 2023-06-14T10:36:12Z

.eslintrc.js

+        {
+            files: ["*.ts"],
+            rules: {
+                "@typescript-eslint/explicit-function-return-type": "error",


А зачем хочешь обязательно тип указывать? TS ведь может самостоятельно корректно определить return type в большинстве кейсов.

Мне кажется явное задание возвращаемого типа в функциях — хорошая практика, такой код очень удобно и приятно читать, даже если это происходит в github или виме не надо анализировать, что тут вернется. Ну и кажется раньше в монорепе был такой конфиг у нас.

Могу убрать, конечно, но я считаю, что это очень полезное правило

Я тоже скорее склоняюсь к explicit аннотациям, именно по причине таких юз кейсов типа ревью, гит истории и тп

DudaGod · 2023-06-14T10:37:45Z

bin/hermione

@@ -1,4 +1,4 @@
 #!/usr/bin/env node
 'use strict';

-var cli = require('../build/cli').run();
+var cli = require('../build/src/cli').run();


А для чего у нас внутри build создается папка src? Я бы ожидал, что она трансформируется в build.

А что в этом плохого? Рома тоже спрашивал этот момент. Такие поинты:

rootDir изменился с ./src на . из-за того, что у нас в коде есть импорты файлов вне src, например, из package.json у нас достается имя пакета и версия (ранее работало, т.к. импорты находились в js файлах)

build/src фигурирует всего в 2-х местах и по факту не влияет ни на пользователя, ни на разработчика. Не вижу в этом какой-то проблемы.

Поэтому оставил так. Однако я всегда открыт для предложений и готов поменять этот момент.

Да, src там появилась потому что появился импорт, который идет вне src. Разницы никакой нет, поэтому предлагаю не заморачиваться

DudaGod · 2023-06-14T10:41:37Z

package.json

@@ -2,14 +2,13 @@
  "name": "hermione",
  "version": "7.0.9",
  "description": "Tests framework based on mocha and wdio",
-  "main": "build/hermione.js",
+  "main": "build/src/index.js",


Тут тоже странно, что src оказывается внутри build

ответил выше

DudaGod · 2023-06-14T10:48:02Z

src/base-hermione.ts

+const PREFIX = packageJson.name + "-";
+
+export abstract class BaseHermione extends AsyncEmitter {
+    protected _interceptors: Interceptor[] = [];


А для чего мы тут нижние подчеркивания оставляем? Чтобы чуть меньше кода изменилось? По идее же они нам больше не нужны?

Уточню, что не предлагаю их обязательно прям сейчас выгашивать.

Почему? Мне кажется нужны, удобно ведь видеть по названию, какие поля публичные, а какие — нет

Я обычно пишу подчеркивания и в TS у не-публичных полей.
Но можно обсудить, я не против поменять

Мне кажется нужны, удобно ведь видеть по названию, какие поля публичные

не, Коля, подчеркивания это олд скул, когда не было private. тебе IDE говорит приватный мембер или нет. Название не должно отображать имплементацию, оно должно отображать смысл (семантику, суть). В названиях не должны отображаться типы например. Или типы аксессоров. И тп.

https://google.github.io/styleguide/tsguide.html#identifiers-underscore-prefix-suffix

вот гугловский стайлгайд например. Я в принципе не видел ТС стайлгайдов, где было бы наоборот

У нас много чего в коде осознанно не соответствует гугловским стайлгайдам. Короче, не совсем согласен с поинтом, это субъективное мнение

Но раз уже 2 человека высказались за выпиливание подчеркиваний — принял, убрал подчеркивания во всех TS файлах

DudaGod · 2023-06-14T10:51:50Z

src/browser-pool/cancelled-error.ts

@@ -0,0 +1,9 @@
+export class CancelledError extends Error {


А почему не export default? Чтобы в js файлах не дописывать .default?

На эту тему есть целый канал в telegram: https://t.me/why_not_export_default

На мой взгляд везде где возможно стоит использовать именованные экспорты и импорты, кроме экспорта главного класса из библиотеки

В js-файлах в любом случае нужно менять импорт. Для default дописывать .default, для именованных .CancelledError

я тоже за именной экспорт

DudaGod · 2023-06-14T11:05:48Z

src/browser/types.ts

+             * "ignoreElements", "tolerance", "antialiasingTolerance", "allowViewportOverflow", "captureElementFromTop",
+             * "compositeImage", "screenshotDelay", "selectorToScroll"
+             */
+            assertView: AssertViewCommand;


Для элемента же это некорректное описание тайпинга. Он же не должен селекторы принимать. Т.е. он работает примерно так:

const elem = await browser.$('.selector'); await elem.assertView('state', opts);

Т.е. похоже нужно экспортить AssertViewBrowserCommand и AssertViewElementCommand

Да, спасибо, что обратил на это внимание! Пропустил этот момент.

src/runner/index.ts

DudaGod · 2023-06-14T11:12:23Z

tsconfig.common.json

@@ -14,6 +15,7 @@
        "skipLibCheck": true,
        "sourceMap": true,
        "strict": true,
-        "target": "es2017"
+        "target": "es2021",


А для чего на es2021 переходить? Лучше код компилится?

Мы поддерживаем node >= 16. es2021 — это как раз node 16. Стал править tsconfig и подумал, нафига компилить старый код. Там же больше кода и полифилов будет, если таргет старый, а при новом больше вещей нативных используется

DudaGod · 2023-06-14T11:12:39Z

tsconfig.common.json

@@ -1,6 +1,7 @@
 {
    "include": ["src"],
    "compilerOptions": {
+        "lib": ["DOM", "es2021"],


А как раньше без этой опции работало?

Раньше те части, которые использовали тайпинги из DOM, были на js. При переписывании на TS понадобилось подключить DOM.

да, ДОМа нет в либе самого языка

…ypings BREAKING CHANGE: package exports have changed

shadowusr force-pushed the users/shadowusr/HERMIONE-920.refactor-types branch 2 times, most recently from 221a8e2 to 06f8cfb Compare June 7, 2023 10:57