fix: eslint #32
fix: eslint #32
Conversation
nabalone
force-pushed
the
eslint
branch
2 times, most recently
from
8a8374b
to
c215dda
Compare
| @@ -60,7 +60,7 @@ export function deepStripDemarcation<T>(demarcated: T): T { | |||
| return demarcated.map((element) => deepStripDemarcation(element)) as T; | |||
| } | |||
| if (typeof demarcated === "object") { | |||
| const newObject = {} as any; | |||
| const newObject: any = {}; | |||
There was a problem hiding this comment.
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?
| @@ -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.
ts-node doesn't work well with node >= 20 :(
TypeStrong/ts-node#1997
paranext/paranext-core@7bb5219a0#diff-840902f027c26280334dfa9c48e1e11c2c1caabd5adb91eb000533dd8b35c235R5
There was a problem hiding this comment.
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
| 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.
memo was not making a noticeable difference in speed now that we lazyload
| 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.
I put adding eslint-plugin-react-hooks into the kanban
| }, | ||
| }, | ||
| ].map((c) => { | ||
| return { ...c, files: ["**/*.{jsx,mjsx,tsx,mtsx}"] }; |
There was a problem hiding this comment.
We can also limit things to folders, i.e. files: ["components/language-chooser/react/**/*/.{jsx,mjsx,tsx,mtsx}"]. Should we talk more about how to setup what eslint rules apply to where?
There was a problem hiding this comment.
Happy to discuss if that's helpful. But applying react rules to react files makes sense.
| @@ -0,0 +1,85 @@ | |||
| // @ts-check | |||
There was a problem hiding this comment.
Maybe we should live-review this file? Let me know if you think that would be helpful
There was a problem hiding this comment.
I don't think it is necessary. But happy to discuss if that helps.
|
Ready for review |
There was a problem hiding this comment.
Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nabalone)
components/language-chooser/common/find-language/package.json line 29 at r1 (raw file):
"// We've tested with 18 and have no reason to believe it won't work with higher versions": "", "engines": { "node": ">=22.11.0"
Is this intentional? I thought we wanted to keep this as low as possible.
Same for the other package.jsons.
If we do want to keep this for some reason, the comments should be updated.
| @@ -0,0 +1,85 @@ | |||
| // @ts-check | |||
There was a problem hiding this comment.
I don't think it is necessary. But happy to discuss if that helps.
| }, | ||
| }, | ||
| ].map((c) => { | ||
| return { ...c, files: ["**/*.{jsx,mjsx,tsx,mtsx}"] }; |
There was a problem hiding this comment.
Happy to discuss if that's helpful. But applying react rules to react files makes sense.
| @@ -60,7 +60,7 @@ export function deepStripDemarcation<T>(demarcated: T): T { | |||
| return demarcated.map((element) => deepStripDemarcation(element)) as T; | |||
| } | |||
| if (typeof demarcated === "object") { | |||
| const newObject = {} as any; | |||
| const newObject: any = {}; | |||
There was a problem hiding this comment.
Reviewable status: 14 of 18 files reviewed, 4 unresolved discussions (waiting on @andrew-polk)
components/language-chooser/common/find-language/package.json line 29 at r1 (raw file):
Previously, andrew-polk wrote…
Is this intentional? I thought we wanted to keep this as low as possible.
Same for the other
package.jsons.If we do want to keep this for some reason, the comments should be updated.
I must have forgotten, why do want to keep it as low as possible again? Our current @typescript-eslint wants at least 18.18 so we can use that
| @@ -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.
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
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions
components/language-chooser/common/find-language/package.json line 29 at r1 (raw file):
Previously, nabalone (Noel) wrote…
I must have forgotten, why do want to keep it as low as possible again? Our current @typescript-eslint wants at least 18.18 so we can use that
Unless I'm mistaken, that is the lowest version of a node a client can use.
We don't want to force libraries to the latest without a reason.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @nabalone)
This change is