-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: eslint #32
fix: eslint #32
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
"macrolanguage", | ||
"Subtag", | ||
"subtags", | ||
"tseslint", | ||
"typecheck" | ||
] | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,11 @@ | |
"main": "./index.js", | ||
"types": "./index.d.ts", | ||
"scripts": { | ||
"langtag-processing": "ts-node-esm ./langtagProcessing.ts", | ||
"langtag-processing": "tsx ./langtagProcessing.ts", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ts-node doesn't work well with node >= 20 :( TypeStrong/ts-node#1997 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if we're going sticking with node 18 for now, I figure we might as well keep this change so that we can upgrade node in the future |
||
"build": "nx vite:build", | ||
"typecheck": "tsc", | ||
"test": "nx vite:test --config vitest.config.ts" | ||
"test": "nx vite:test --config vitest.config.ts", | ||
"lint": "eslint ." | ||
}, | ||
"dependencies": { | ||
"fuse.js": "^7.0.0", | ||
|
@@ -20,11 +21,11 @@ | |
"devDependencies": { | ||
"@nx/vite": "^19.1.2", | ||
"@types/node": "^20.16.11", | ||
"ts-node": "^10.9.2", | ||
"tsx": "^4.19.2", | ||
"typescript": "^5.2.2" | ||
}, | ||
"// We've tested with 18 and have no reason to believe it won't work with higher versions": "", | ||
"engines": { | ||
"node": ">=18.0.0" | ||
"node": ">=18.18" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,15 +6,14 @@ import { | |
OptionCardPropsWithoutColors, | ||
} from "./OptionCard"; | ||
import { ILanguage } from "@ethnolib/find-language"; | ||
import { memo } from "react"; | ||
import { PartiallyBoldedTypography } from "./PartiallyBoldedTypography"; | ||
import { COLORS } from "./colors"; | ||
|
||
const COMMA_SEPARATOR = ", "; | ||
|
||
export const LanguageCard: React.FunctionComponent< | ||
{ languageCardData: ILanguage } & OptionCardPropsWithoutColors | ||
> = memo(({ languageCardData, ...partialOptionCardProps }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. memo was not making a noticeable difference in speed now that we lazyload |
||
> = ({ languageCardData, ...partialOptionCardProps }) => { | ||
const optionCardProps = { | ||
...partialOptionCardProps, | ||
backgroundColorWhenNotSelected: COLORS.white, | ||
|
@@ -91,4 +90,4 @@ export const LanguageCard: React.FunctionComponent< | |
)} | ||
</OptionCard> | ||
); | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
// @ts-check | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should live-review this file? Let me know if you think that would be helpful There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is necessary. But happy to discuss if that helps. |
||
|
||
import eslint from "@eslint/js"; | ||
import tseslint from "typescript-eslint"; | ||
import reactPlugin from "eslint-plugin-react"; | ||
import eslintConfigPrettier from "eslint-config-prettier"; | ||
import eslintPluginPrettierRecommended from "eslint-plugin-prettier/recommended"; | ||
// import eslintPluginReactHooks from "eslint-plugin-react-hooks"; | ||
// TODO future work: eslint-plugin-react-hooks support for flat-config looks like it's on it's way: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put adding eslint-plugin-react-hooks into the kanban |
||
// https://github.com/facebook/react/pull/30774. I say we wait for it to be released then can easily add it | ||
|
||
export default [ | ||
{ | ||
ignores: ["**/node_modules/**/*", "**/dist/**"], | ||
}, | ||
eslint.configs.recommended, | ||
eslintConfigPrettier, // disables eslint rules that could conflict with prettier | ||
eslintPluginPrettierRecommended, | ||
...tseslint.config( | ||
// tseslint.config() is a helper function giving autocomplete and documentation https://typescript-eslint.io/packages/typescript-eslint#config | ||
// not sure if we want this, we can get rid of it if it causes any trouble | ||
...tseslint.configs.recommended, | ||
{ | ||
files: ["**/*.{ts,tsx}"], | ||
plugins: { | ||
"@typescript-eslint": tseslint.plugin, | ||
}, | ||
rules: { | ||
// Copied from BloomDesktop | ||
"prettier/prettier": "off", | ||
"no-var": "warn", | ||
"prefer-const": "warn", | ||
"no-useless-escape": "off", | ||
"no-irregular-whitespace": [ | ||
"error", | ||
{ skipStrings: true, skipTemplates: true }, | ||
], | ||
"no-warning-comments": [ | ||
1, | ||
{ terms: ["nocommit"], location: "anywhere" }, | ||
], | ||
// Downgraded from error to warnings | ||
"@typescript-eslint/no-empty-function": "warn", | ||
"@typescript-eslint/no-empty-interface": "warn", | ||
"@typescript-eslint/no-explicit-any": "warn", | ||
"@typescript-eslint/no-unused-vars": [ | ||
"warn", | ||
{ argsIgnorePattern: "^_", varsIgnorePattern: "^_" }, | ||
], | ||
"@typescript-eslint/no-var-requires": "warn", | ||
"no-case-declarations": "warn", | ||
"prefer-rest-params": "warn", | ||
"prefer-spread": "warn", | ||
eqeqeq: ["warn", "always"], | ||
// Disabled | ||
"@typescript-eslint/ban-types": "off", // Record<string, never> is not intuitive for us compared to {} | ||
"@typescript-eslint/no-inferrable-types": "off", // not worth worrying about (not even convinced it's a problem at all) | ||
}, | ||
} | ||
), | ||
|
||
// React configs to be applied to react files only | ||
...[ | ||
reactPlugin.configs.flat?.recommended, | ||
reactPlugin.configs.flat?.["jsx-runtime"], | ||
{ | ||
settings: { | ||
react: { | ||
version: "17.0", // React version. Should match the version which the react packages are using. Currently cannot be automatically detected because we don't have react installed at the root (here) | ||
}, | ||
}, | ||
plugins: { | ||
react: reactPlugin, | ||
}, | ||
rules: { | ||
// Copied from BloomDesktop | ||
"react/no-unknown-property": ["error", { ignore: ["css"] }], // allow emotion css: https://emotion.sh/docs/eslint-plugin-react | ||
"react/no-unescaped-entities": "off", // Complains about some special chars that sort of work, but due to the burden that enocded chars present to localizers, we'd prefer not to encode them if not necessary. | ||
"react/prop-types": "off", // Seems to require validation on the props parameter itself, but Typescript can already figure out the types through annotations in different places, seems unnecessary | ||
}, | ||
}, | ||
].map((c) => { | ||
return { ...c, files: ["**/*.{jsx,mjsx,tsx,mtsx}"] }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can also limit things to folders, i.e. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to discuss if that's helpful. But applying react rules to react files makes sense. |
||
}), | ||
]; |
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.
Are these anys okay? Also on line 65. Would it be better to create a new interface e.g.
`
interface Foo {
[key: string]: T;
} and if so what should it be called? Seems like a lot of code for something so small?
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.
Ok with me.