-
Notifications
You must be signed in to change notification settings - Fork 7
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
refactor(arui-scripts): refactoring linter rules #169
Conversation
|
packages/arui-scripts/.eslintrc.js
Outdated
@@ -5,7 +5,7 @@ module.exports = { | |||
tsconfigRootDir: __dirname, | |||
project: ['./tsconfig.eslint.json'], | |||
}, | |||
ignorePatterns: ['./src/templates/dockerfile-compiled.template.ts', '.turbo'], | |||
ignorePatterns: ['./src/templates/dockerfile-compiled.template.ts', '.turbo','release.config.js'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ну получается что мы фактически тупо игнорируем вообще все просто ради того, чтобы выпилить игнор одного правила. Как то это не правильно. Я бы просто оставил тот disable что есть в release.config и удалил оттуда todo - толку от этого будет больше.
Но вообще semantic release в проекте не используется и можно бы его вообще выпилить. Но это уже скорее отдельная задача
@@ -34,8 +34,7 @@ describe('update-with-presets', () => { | |||
overridesPath: ['package-overrides-path.js'], | |||
} as AppContext; | |||
|
|||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
const updatedConfig = updateWithPresets(baseConfig, context); | |||
updateWithPresets(baseConfig, context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а куда переменная updatedConfig
делась? Или она не использовалась?
|
||
const presetsConfigPath = tryResolve(`${config.presets}/arui-scripts.config`, { | ||
let appConfig = cloneDeep(config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
раньше копирования не было, зачем копируем?
// eslint-disable-next-line no-continue | ||
continue; | ||
} | ||
const rules = config.module!.rules as webpack.RuleSetRule[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
лучше оставить прежнее поведение, оно по идее отсекает типы
c2d5c33
to
83bf584
Compare
88dde8e
to
a79fbbe
Compare
2bb9b35
to
0392064
Compare
} catch (error) { | ||
// empty error | ||
} | ||
import * as brotliSize from 'brotli-size'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
здесь лучше сделать import { sync } from 'brotli-size';
если такое возможно, тк есть мнение что так tree shaking лучше сработает
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tree shaking в консольной утилите не то чтоб сильно актуален. Я за то чтоб оставить либо текущую запись, либо переделать на такую же, как и у gzip-size, потому что что такое функция sync - понять совсем не реально, а что такое brotlySize.sync - вполне
paths: [context.cwd], | ||
}); | ||
const presetsOverridesPath = tryResolve(`${config.presets}/arui-scripts.overrides`, { | ||
const presetsOverridesPath = tryResolve(`${appConfig.presets}/arui-scripts.overrides`, { | ||
paths: [context.cwd], | ||
}); | ||
|
||
if (presetsConfigPath) { | ||
// eslint-disable-next-line import/no-dynamic-require, @typescript-eslint/no-var-requires, global-require | ||
let presetsSettings = require(presetsConfigPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require тут можно динамическим импортом заменить, тогда eslint комменты можно будет снести
@@ -75,7 +74,7 @@ type OverrideFunction< | |||
> = ( | |||
config: Overrides[K], | |||
appConfig: AppContextWithConfigs, | |||
additionalArgs: AdditionalArgs, | |||
additionalArgs: AdditionalArgs | unknown, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не надо тут unknown. Обсуждали в чате - этот тип выставляется пользователям. Сейчас он точно говорит что за тип ожидать при использовании оверрайда.
Пример использования на проекте
const override: OverrideFile = {
webpackClient: (config, appConfig, { createSingleClientWebpackConfig }) => {
return createSingleClientWebpackConfig('src/index', 'something');
}
};
С типами без unknown - все работает. Если добавить unknown - ts будет ругаться. Нужно чтобы не ругался.
Если при этом никак не обойтись без использования например any в коде библиотеки - это ок, правильность и удобство типов для потребителей важнее чем прохождение правил линтера в библиотеке.
No description provided.