-
Notifications
You must be signed in to change notification settings - Fork 13
migrate to eslint 9 #83
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
base: main
Are you sure you want to change the base?
Conversation
| ### Changed | ||
|
|
||
| - migrated to eslint 9 flat config | ||
| - removed obsolete storybook |
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.
I will readd the storybook in a next pr with an updated of all the used packages... the current storybook version was still using the old before tanstack table and was therefor not useful anymore at all.
commit: |
| virtualizer?: Virtualizer<HTMLDivElement, Element>; | ||
| } | ||
|
|
||
| // eslint-disable-next-line complexity |
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.
the complexity is really low in the eslint-config-neolution, is there a reason to have 12 in the package instead of the default 20?
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.
it was initially taken over from the config we had in whitelabel:
https://github.com/neolution-ch/neolution-dotnet-templates/pull/230/files#diff-c853350721291cfdc71a00324eb7d2ab43b4db04f74afefc6ff5ac4b4f3a7f4f
but i will take it on the list to discuss and maybe increase directly in the package 💪
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.
discussed and adjusted to the default values 💪
|
|
||
| // If we active the manual filtering, we have to unset the filter function, else it still does automatic filtering | ||
| if (manualFiltering) columns.forEach((x) => (x.filterFn = undefined)); | ||
| if (manualFiltering) for (const x of columns) x.filterFn = undefined; |
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.
there's no rule to have these logics with curly braces blocks?
nesting so much with the "inline block" might be dangerous and less readable, no?
my suggestion is to have it like this always:
if (manualFiltering) {
for (const x of columns) {
x.filterFn = undefined;
}
}
are you aware of a rule like this? at least to limit to only allow 1 nested inline block?
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.
seems there's no way to limit to only 1 nested, would be interesting in big projects to see what happens by enabling this:
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.
mhmm that is actually interesting, honestly this part was autoupdated by eslint, so I didnt put too much thoughts into it... but we also have a rule like this in c#, maybe I can take that as another point to discuss.. you would be for enabling this rule, right?
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.
I checked this with Timothy and added to my tech tasks, to check this rule in a big current project (for example hbl), to figure out, how well the configuration and especially the autofix works :) will keep you up to date as soon as I have a result there.
| "eslint-plugin-react-hooks": "^4.6.0", | ||
| "eslint-plugin-storybook": "^0.6.12", | ||
| "eslint": "^9.37.0", | ||
| "eslint-plugin-storybook": "^0.10.0", |
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.
I guess many libraries will need the eslint-plugin-storybook, is not worth to have it directly in eslint-config-neolution?
don't think those few kb are a problem for projects not actually using it
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.
The storybooks were super neglejected in most projects, in this one here, it still had the reference to the datatable before we even used tanstack 😆 That's why initially it was not included in the package rule set.. but now with the possibility to configure the rules, it would definitely be an option and nicer... I will also take that on the list to discuss.. I think we should discuss in general, if out packages should include a story book or not (and then should also be maintained)
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.
storybook will be added to the eslint package, task is on my list :)
| import { getFilterValue, setFilterValue } from "../utils/customFilterMethods"; | ||
| import { useVirtualizer, Virtualizer } from "@tanstack/react-virtual"; | ||
| import { useRef } from "react"; | ||
|
|
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.
@neotob The empty line could be removed at all :)
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.
@manni497 random newline removed :D
| export { ReactDataTable, ReactDataTableProps }; | ||
| export { ReactDataTable }; | ||
|
|
||
| export { ReactDataTableProps } from "./ReactDataTableProps"; |
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.
It was a strange implementation before, make sense to add the ReactDataTableProps directly to the index.ts instead?
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.
i was thinking the same, but I didn't dare to break anything just because of a eslint upgrade :D
|
|
||
| export { useReactDataTableProps } from "./useReactDataTableProps"; | ||
|
|
||
| export { useReactDataTableResult } from "./useReactDataTableResult"; |
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.
Same here, makes sense to add both files to the index.ts?
| export { useReactDataTableState, useReactDataTableStateProps, useReactDataTableStateResult }; | ||
| export { useReactDataTableState }; | ||
|
|
||
| export { useReactDataTableStateProps } from "./useReactDataTableStateProps"; |
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.
Same as previous comment :)
src/react-table.d.ts
Outdated
| @@ -1,4 +1,5 @@ | |||
| /* eslint-disable @typescript-eslint/no-unused-vars */ | |||
| /* eslint-disable import/no-duplicates */ | |||
| import "@tanstack/react-table"; | |||
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.
@neotob Have you tried to remove this import?
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.
same here, not sure what this did, but at least the pipelines are still happy :P
rollup.config.mjs
Outdated
| @@ -1,4 +1,5 @@ | |||
| import commonjs from "@rollup/plugin-commonjs"; | |||
| // eslint-disable-next-line import/no-named-as-default | |||
| import nodeResolve from "@rollup/plugin-node-resolve"; | |||
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.
I think we found a solution for this?
You should use a different name or import default as :)
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.
used another name thanks :)
No description provided.