-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Update eslint deps for compatibility with new TS version #18024
Conversation
packages/eslint-plugin/package.json
Outdated
@@ -9,29 +9,29 @@ | |||
}, | |||
"license": "MIT", | |||
"scripts": { | |||
"lint": "eslint --ext .js src" | |||
"build": "tsc --noEmit", |
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.
This package is still written in JS (so other packages in the repo can use it without it being compiled first), but I added this step to ensure type checking.
@@ -21,7 +25,7 @@ const config = { | |||
'import/resolver': { | |||
typescript: { | |||
alwaysTryTypes: true, // always try to resolve types under `<root>@types` directory | |||
directory: process.cwd(), | |||
project: [path.join(process.cwd(), 'tsconfig.json'), path.join(gitRoot, 'tsconfig.json')].filter(fs.existsSync), |
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 directory
option is deprecated in favor of project
command += e.key; | ||
hotkeys.hasOwnProperty(command) && hotkeys[command](); | ||
}, | ||
[hotkeys], |
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.
An improvement to the hooks lint rule pointed out that since the hotkeys
object is re-created on every render, this useCallback
will run on every render too, so there's no point in having it (and I couldn't see a straightforward way to avoid hotkeys
being re-created)
@@ -14,7 +14,7 @@ export type ListProps = { | |||
export const List: React.FunctionComponent<ListProps> = ({ onDragStart, style }) => { | |||
const [filter, setFilter] = React.useState<string>(''); | |||
|
|||
const filterRegexp = new RegExp(filter, 'i'); | |||
const filterRegexp = React.useMemo(() => new RegExp(filter, 'i'), [filter]); |
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 improved hooks rule pointed out that this regex being created on every render was causing some later hook to re-run on every render
@@ -10,9 +10,13 @@ | |||
}, | |||
{ | |||
"files": "src/**/*.{ts,tsx}", | |||
"parserOptions": { | |||
"lib": ["es2015", "dom"] |
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.
This is now necessary to avoid warnings about DOM types such as ClientRect
being undefined. Similar with the JSX
change below.
@@ -166,6 +166,9 @@ export const Datepicker: ComponentWithAs<'div', DatepickerProps> & | |||
setStart(); | |||
const inputRef = React.useRef<HTMLElement>(); | |||
|
|||
// FIXME: This object is created every render, causing a cascade of useCallback/useEffect re-runs. |
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.
This is a potentially significant performance problem where a proper fix wasn't obvious to me without more detailed knowledge of the desired component behavior. @pompomon FYI, might be worth coming back with a fix later.
@@ -12,6 +12,8 @@ export interface IThemeProviderProps { | |||
* for a given scheme name. | |||
* | |||
* @param providers - Injected providers for accessing theme data and providing it via a Customizer component. | |||
* @deprecated This is an old ThemeProvider implementation. New code should use the ThemeProvider exported from |
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.
Deprecated this old foundation ThemeProvider to reduce potential for confusion when sorting through auto-suggested imports
@@ -137,32 +151,32 @@ const Example = () => ( | |||
|
|||
export const Light = () => ( | |||
<Customizer {...AzureCustomizationsLight}> | |||
<Fabric applyThemeToBody> | |||
<ThemeProvider applyTo="body"> |
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.
TODO, need to verify this works
* @param DefaultedProps - The keys of Props that will always have a default value provided | ||
* @template Props - The component's Props type | ||
* @template RefType - The type of the state.ref property; e.g. `React.Ref<HTMLElement>` | ||
* @template ShorthandPropNames - The keys of Props that correspond to ShorthandProps |
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.
This was flagged because the ShorthandProps
type parameter shadowed the type ShorthandProps
defined earlier in the file. Open to other suggestions for new names. (Also type params should be documented with @template
not @param
.)
@behowell FYI
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.
Thanks for fixing this!
@@ -140,10 +140,10 @@ export class List<T = any> extends React.Component<IListProps<T>, IListState<T>> | |||
private _scrollTop: number; | |||
private _pageCache: IPageCache<T>; | |||
|
|||
public static getDerivedStateFromProps<T = any>( |
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 already a T
defined at the class level. Open to other naming suggestions.
79b4642
to
281ce5a
Compare
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 68429db4db11d965e18e04c720d8fd83904c9742 (build) |
281ce5a
to
40848bf
Compare
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 869 | 817 | 5000 | |
BaseButton | mount | 857 | 897 | 5000 | |
Breadcrumb | mount | 2533 | 2613 | 1000 | |
ButtonNext | mount | 475 | 482 | 5000 | |
Checkbox | mount | 1575 | 1557 | 5000 | |
CheckboxBase | mount | 1255 | 1280 | 5000 | |
ChoiceGroup | mount | 4591 | 4639 | 5000 | |
ComboBox | mount | 975 | 963 | 1000 | |
CommandBar | mount | 9978 | 9948 | 1000 | |
ContextualMenu | mount | 5939 | 6033 | 1000 | |
DefaultButton | mount | 1084 | 1105 | 5000 | |
DetailsRow | mount | 3685 | 3710 | 5000 | |
DetailsRowFast | mount | 3644 | 3653 | 5000 | |
DetailsRowNoStyles | mount | 3441 | 3383 | 5000 | |
Dialog | mount | 2106 | 2167 | 1000 | |
DocumentCardTitle | mount | 136 | 142 | 1000 | |
Dropdown | mount | 3157 | 3138 | 5000 | |
FocusTrapZone | mount | 1737 | 1747 | 5000 | |
FocusZone | mount | 1815 | 1764 | 5000 | |
IconButton | mount | 1729 | 1696 | 5000 | |
Label | mount | 330 | 339 | 5000 | |
Layer | mount | 1740 | 1752 | 5000 | |
Link | mount | 468 | 468 | 5000 | |
MakeStyles | mount | 1784 | 1760 | 50000 | |
MenuButton | mount | 1394 | 1425 | 5000 | |
MessageBar | mount | 1978 | 2071 | 5000 | |
Nav | mount | 3229 | 3196 | 1000 | |
OverflowSet | mount | 994 | 1022 | 5000 | |
Panel | mount | 2010 | 2104 | 1000 | |
Persona | mount | 797 | 808 | 1000 | |
Pivot | mount | 1385 | 1386 | 1000 | |
PrimaryButton | mount | 1234 | 1246 | 5000 | |
Rating | mount | 7429 | 7377 | 5000 | |
SearchBox | mount | 1285 | 1292 | 5000 | |
Shimmer | mount | 2448 | 2499 | 5000 | |
Slider | mount | 1967 | 1898 | 5000 | |
SpinButton | mount | 4906 | 4928 | 5000 | |
Spinner | mount | 409 | 403 | 5000 | |
SplitButton | mount | 3147 | 3038 | 5000 | |
Stack | mount | 490 | 484 | 5000 | |
StackWithIntrinsicChildren | mount | 1472 | 1501 | 5000 | |
StackWithTextChildren | mount | 4402 | 4376 | 5000 | |
SwatchColorPicker | mount | 10061 | 9984 | 5000 | |
Tabs | mount | 1357 | 1366 | 1000 | |
TagPicker | mount | 2399 | 2402 | 5000 | |
TeachingBubble | mount | 11662 | 11611 | 5000 | |
Text | mount | 402 | 392 | 5000 | |
TextField | mount | 1335 | 1373 | 5000 | |
ThemeProvider | mount | 1174 | 1134 | 5000 | |
ThemeProvider | virtual-rerender | 631 | 591 | 5000 | |
ThemeProviderNext | mount | 6805 | 6769 | 5000 | |
Toggle | mount | 793 | 794 | 5000 | |
buttonNative | mount | 110 | 119 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
Perf tests with no regressions
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
BoxMinimalPerf.default | 365 | 328 | 1.11:1 |
FormMinimalPerf.default | 396 | 368 | 1.08:1 |
RefMinimalPerf.default | 240 | 223 | 1.08:1 |
TableMinimalPerf.default | 404 | 378 | 1.07:1 |
ChatWithPopoverPerf.default | 362 | 342 | 1.06:1 |
LayoutMinimalPerf.default | 370 | 350 | 1.06:1 |
DividerMinimalPerf.default | 359 | 341 | 1.05:1 |
FlexMinimalPerf.default | 286 | 273 | 1.05:1 |
RadioGroupMinimalPerf.default | 438 | 419 | 1.05:1 |
CarouselMinimalPerf.default | 464 | 448 | 1.04:1 |
ListWith60ListItems.default | 637 | 610 | 1.04:1 |
PortalMinimalPerf.default | 162 | 156 | 1.04:1 |
ReactionMinimalPerf.default | 390 | 374 | 1.04:1 |
SliderMinimalPerf.default | 1564 | 1509 | 1.04:1 |
AttachmentMinimalPerf.default | 152 | 147 | 1.03:1 |
DropdownManyItemsPerf.default | 677 | 659 | 1.03:1 |
ImageMinimalPerf.default | 367 | 355 | 1.03:1 |
ItemLayoutMinimalPerf.default | 1276 | 1238 | 1.03:1 |
StatusMinimalPerf.default | 672 | 652 | 1.03:1 |
TextAreaMinimalPerf.default | 471 | 459 | 1.03:1 |
ButtonSlotsPerf.default | 544 | 534 | 1.02:1 |
DropdownMinimalPerf.default | 3023 | 2969 | 1.02:1 |
HeaderMinimalPerf.default | 353 | 346 | 1.02:1 |
ListMinimalPerf.default | 489 | 479 | 1.02:1 |
MenuMinimalPerf.default | 834 | 817 | 1.02:1 |
ProviderMinimalPerf.default | 964 | 943 | 1.02:1 |
AnimationMinimalPerf.default | 405 | 400 | 1.01:1 |
ButtonMinimalPerf.default | 167 | 165 | 1.01:1 |
ButtonOverridesMissPerf.default | 1662 | 1638 | 1.01:1 |
ChatDuplicateMessagesPerf.default | 292 | 288 | 1.01:1 |
ChatMinimalPerf.default | 597 | 590 | 1.01:1 |
HeaderSlotsPerf.default | 743 | 737 | 1.01:1 |
ProviderMergeThemesPerf.default | 1697 | 1677 | 1.01:1 |
IconMinimalPerf.default | 600 | 593 | 1.01:1 |
TableManyItemsPerf.default | 1860 | 1846 | 1.01:1 |
TextMinimalPerf.default | 333 | 331 | 1.01:1 |
CheckboxMinimalPerf.default | 2685 | 2691 | 1:1 |
InputMinimalPerf.default | 1226 | 1226 | 1:1 |
ListCommonPerf.default | 612 | 610 | 1:1 |
ListNestedPerf.default | 544 | 546 | 1:1 |
PopupMinimalPerf.default | 548 | 547 | 1:1 |
SkeletonMinimalPerf.default | 343 | 342 | 1:1 |
ToolbarMinimalPerf.default | 914 | 916 | 1:1 |
CardMinimalPerf.default | 532 | 535 | 0.99:1 |
DatepickerMinimalPerf.default | 5310 | 5374 | 0.99:1 |
DialogMinimalPerf.default | 716 | 721 | 0.99:1 |
EmbedMinimalPerf.default | 4036 | 4097 | 0.99:1 |
LabelMinimalPerf.default | 371 | 373 | 0.99:1 |
CustomToolbarPrototype.default | 3700 | 3728 | 0.99:1 |
TreeMinimalPerf.default | 749 | 759 | 0.99:1 |
GridMinimalPerf.default | 318 | 325 | 0.98:1 |
MenuButtonMinimalPerf.default | 1488 | 1511 | 0.98:1 |
SegmentMinimalPerf.default | 334 | 340 | 0.98:1 |
SplitButtonMinimalPerf.default | 3611 | 3699 | 0.98:1 |
TooltipMinimalPerf.default | 936 | 952 | 0.98:1 |
VideoMinimalPerf.default | 621 | 637 | 0.97:1 |
AttachmentSlotsPerf.default | 1108 | 1153 | 0.96:1 |
LoaderMinimalPerf.default | 659 | 685 | 0.96:1 |
AlertMinimalPerf.default | 253 | 265 | 0.95:1 |
AvatarMinimalPerf.default | 193 | 206 | 0.94:1 |
AccordionMinimalPerf.default | 140 | 151 | 0.93:1 |
RosterPerf.default | 1084 | 1168 | 0.93:1 |
TreeWith60ListItems.default | 167 | 186 | 0.9:1 |
"@typescript-eslint/eslint-plugin": "4.22.0", | ||
"@typescript-eslint/experimental-utils": "4.22.0", | ||
"@typescript-eslint/parser": "4.22.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 forced these resolutions because eslint-plugin-deprecation
, the web-components eslint config, and maybe a couple other things still use older versions of the deps. It's not ideal but appears to be working okay at the moment.
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.
regarding that deprecation plugin, this is the reason why? https://github.com/gund/eslint-plugin-deprecation/blob/master/package.json#L27
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit af4a434:
|
b00a366
to
ad09109
Compare
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
Pull request checklist
$ yarn change
Description of changes
Update eslint-related dependencies to be compatible with TS 4.1, including proper parsing for
import type
/export type
.After this change, it will probably be necessary for everyone to delete their local eslint caches (
yarn rimraf '{apps,packages}/*/.eslintcache'
or similar) to avoid some weird false positives.Thanks to one or more of the updated deps (hard to say which), usage of React components is now flagged! 🎉 However, this meant that a bunch more rule disables had to be added, mainly for usage of
Fabric
andCustomizer
. Where it made sense and was clearly safe, I went ahead and changed these to useThemeProvider
, but in other cases I just disabled the rule.There are also a few other interesting changes that I'll comment on inline.
Related references:
@typescript-eslint/*
v4 release notes@typescript-eslint/*
v3 release notes