Rewrite package on typescript #23

OldSkyTree · 2022-02-12T20:19:17Z

No description provided.

lib/lazy.ts

lib/core.ts

OldSkyTree · 2022-02-14T10:37:52Z

Запушил с переименованием файлов, теперь если смотреть по коммитам, то можно смотреть только на изменения, а не на весь файл

xrsd · 2022-10-24T14:22:40Z

README.md

@@ -8,6 +8,7 @@ Config is described with a combination of a functions:
 var parser = root(section({
    system: section({
        parallelLimit: option({
+            defaultValue: 0,


0 – не самый удачный пример дефолта. Кстати, в гермионе у этой опции дефолт Infinity.

xrsd · 2022-10-24T14:27:19Z

lib/errors.ts

-class UnknownKeysError extends Error {
-    constructor(keys) {
+export class UnknownKeysError extends Error {
+    keys: Array<string>;


А это зачем?

xrsd · 2022-11-03T07:17:23Z

lib/types/locator.ts

+
+export interface Locator<Options> extends Node<Options> {
+    nested<Key extends keyof Options>(key: Key): Locator<Options[Key]>;
+    resetOption(newOption: Options): Locator<Options>;


Как-то не вяжется, что аргумент называется newOption, а тип Options – во множественном числе (как будто это массив).

xrsd · 2022-11-03T08:38:20Z

package.json

  },
  "dependencies": {
    "lodash": "^4.17.4"
  },
  "scripts": {
+    "build": "tsc --build",
+    "prepublishOnly": "npm run build",


Здесь тоже вопрос, почему prepublishOnly, а не prepack?

https://github.com/gemini-testing/hermione-storybook/blob/master/package.json#L16

xrsd · 2022-11-03T08:39:28Z

Кстати, еще вопрос: а не надо переименовывать папку lib в src?

DudaGod

В PR "Rewrite package on typescript" ожидаешь увидеть полный переезд на TS. Но при этом тесты остались на js. И структура пакета отличается от привычной в TS. Давай это исправим.

DudaGod · 2022-11-03T09:10:46Z

.eslintrc.js

-    extends: 'gemini-testing',
+    extends: ['gemini-testing', 'plugin:@typescript-eslint/recommended'],
+    parser: '@typescript-eslint/parser',
+    plugins: ['@typescript-eslint'],
    root: true


Немного странная последовательность опций. Я бы в такой последовательности описывал - https://github.com/gemini-testing/hermione-storybook/blob/master/.eslintrc.js

DudaGod · 2022-11-03T09:15:11Z

tsconfig.json

+{
+  "extends": "@tsconfig/recommended/tsconfig.json",
+  "include": ["lib"],
+  "exclude": ["build", "test"],


Странный exclude. У тебя же эти папки в include не попадают вообще.

Ну и почему сразу не сделать по нормальному? Т.е. не переименовать lib в src и в этот же src положить тесты?

DudaGod · 2022-11-03T09:16:06Z

package.json

  },
  "dependencies": {
    "lodash": "^4.17.4"
  },
  "scripts": {
+    "build": "tsc --build",
+    "prepublishOnly": "npm run build",
+    "pretest": "npm run build",


Зачем на каждый test запускать сборку?

DudaGod · 2022-11-03T09:16:32Z

tsconfig.json

@@ -0,0 +1,15 @@
+{
+  "extends": "@tsconfig/recommended/tsconfig.json",


А что ты используешь из этого конфига то? Кажется, что он вообще не нужен

DudaGod · 2022-11-03T09:17:00Z

tsconfig.json

+  "compilerOptions": {
+    "target": "ES2019",
+    "outDir": "build",
+    "allowJs": true,


Ты же пакет переписал на ts. Зачем тут эта опция?

DudaGod · 2022-11-03T09:17:58Z

tsconfig.json

+  "include": ["lib"],
+  "exclude": ["build", "test"],
+  "compilerOptions": {
+    "target": "ES2019",


А какие ты фичи юзаешь из es2019? Спрашиваю так как обычно используем es2017

DudaGod · 2022-11-03T09:18:53Z

.travis.yml

@@ -3,4 +3,5 @@ node_js:
 - '6'
 - '8'
 script:
+- npm build
 - npm test


А здесь у тебя за счет pretest получится двойной запуск build скрипта.

DudaGod · 2022-11-03T09:19:50Z

lib/types/option.ts

+}
+
+/* eslint-disable @typescript-eslint/no-explicit-any */
+export interface OptionParserConfig<Value, MappedValue, Result = any> {


Не понял а для чего ты тут столько any налупил? Давай хотя бы unknown юзать.

OldSkyTree force-pushed the FEI-17917 branch from 41ef2f1 to 751efda Compare February 12, 2022 20:22

OldSkyTree mentioned this pull request Feb 12, 2022

Rewrite package on typescript gemini-testing/gemini-core#108

Open

OldSkyTree force-pushed the FEI-17917 branch 3 times, most recently from 149df46 to 2afb73b Compare February 12, 2022 20:51

xrsd reviewed Feb 13, 2022

View reviewed changes

lib/lazy.ts Outdated Show resolved Hide resolved

lib/lazy.ts Outdated Show resolved Hide resolved

lib/core.ts Show resolved Hide resolved

OldSkyTree force-pushed the FEI-17917 branch from 2afb73b to 1178c03 Compare February 14, 2022 10:36

OldSkyTree force-pushed the FEI-17917 branch 3 times, most recently from 2d637b1 to fda2f4b Compare September 12, 2022 19:28

oldskytree added 4 commits September 12, 2022 22:46

chore!: support nodejs from 12 version and above

c7d1c05

chore: rename files *.js to *.ts

415554c

chore: rewrite package on typescript

01c2536

build: configure eslint for typescript

a5f33b9

OldSkyTree force-pushed the FEI-17917 branch from fda2f4b to a5f33b9 Compare September 12, 2022 19:46

xrsd approved these changes Nov 3, 2022

View reviewed changes

xrsd reviewed Nov 3, 2022

View reviewed changes

DudaGod requested changes Nov 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite package on typescript #23

Rewrite package on typescript #23

OldSkyTree commented Feb 12, 2022

OldSkyTree commented Feb 14, 2022

xrsd Oct 24, 2022

xrsd Oct 24, 2022

xrsd Nov 3, 2022

xrsd Nov 3, 2022

xrsd commented Nov 3, 2022

DudaGod left a comment

DudaGod Nov 3, 2022

DudaGod Nov 3, 2022

DudaGod Nov 3, 2022

DudaGod Nov 3, 2022

DudaGod Nov 3, 2022

DudaGod Nov 3, 2022

DudaGod Nov 3, 2022

DudaGod Nov 3, 2022

		@@ -0,0 +1,15 @@
		{
		"extends": "@tsconfig/recommended/tsconfig.json",

Rewrite package on typescript #23

Are you sure you want to change the base?

Rewrite package on typescript #23

Conversation

OldSkyTree commented Feb 12, 2022

OldSkyTree commented Feb 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrsd commented Nov 3, 2022

DudaGod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment