Clean up all lint warnings #1199

snowystinger · 2020-10-28T00:16:05Z

Our lint warnings are getting away from us, this is a start to clean that up so that we can catch meaningful problems.
Add comments explaining why some are disabled/not needed

Closes

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

adobe-bot · 2020-10-28T00:29:01Z

Build successful! 🎉

adobe-bot · 2020-10-28T00:31:14Z

Build successful! 🎉

LFDanLu · 2020-11-04T19:11:48Z

packages/@react-spectrum/utils/src/Slots.tsx

+  let stableEmptyObject = useRef({});
+  let parentSlots = useContext(SlotContext) || stableEmptyObject.current;


Mind giving me the rundown on this change? Any particular reason for going with this approach rather than moving parentSlots into the useMemo below?

you mean like this instead?

let parentSlots = useContext(SlotContext); let {slots = {}, children} = props; // Merge props for each slot from parent context and props let value = useMemo(() => { let slotObj = parentSlots || {} Object.keys(slotObj) .concat(Object.keys(slots)) .reduce((o, p) => ({ ...o, [p]: mergeProps(slotObj[p]) || {}, slots[p] || {})}), {}) , [parentSlots, slots]);

I'm fine doing that too, it's probably slightly better

so I tried that, and broke like every test... I'm not sure what went wrong, so sticking with stableEmptyObject for the moment

Why is stableEmptyObject used instead of {}? things like that make me nervous that something more important is wrong.

ktabors

LGTM

adobe-bot · 2020-11-11T00:38:51Z

Build successful! 🎉

…lean-up-lint-warnings

adobe-bot · 2020-12-05T01:26:22Z

Build successful! 🎉

snowystinger · 2020-12-05T02:00:12Z

packages/@react-aria/overlays/src/useOverlayPosition.ts

  }, deps);

  // Update position when anything changes
+  // eslint-disable-next-line react-hooks/exhaustive-deps


Out of curiosity, is it just not checking in deps? or is deps actually missing some?
Lets add a comment when we // disable so that we know why we're ignoring it. It looks like you've started doing that a bit, so that's good. We should make sure every disable of this rule has an explanation

For this one, the linter states " React Hook useCallback was passed a dependency list that is not an array literal" and I tried facebook/react#14476 (comment) w/ the JSON.stringify but it broke a lot of stuff so I opted for disabling it. I'll go ahead and put an explanation though, good idea

actually, looking at it again maybe I can do away with the dep array since the useEffect below can have a dep on updatePosition

also changed useDomRef to use useUnwrapDOMRef so that it does not infinite loop

adobe-bot · 2020-12-07T21:03:13Z

Build successful! 🎉

snowystinger · 2020-12-07T22:08:53Z

packages/@react-spectrum/combobox/src/ComboBox.tsx

@@ -65,8 +67,8 @@ const ComboBoxBase = React.forwardRef(function ComboBoxBase<T extends object>(pr
    {
      ...props,
      keyboardDelegate: layout,
-      buttonRef: unwrapDOMRef(buttonRef),
-      popoverRef: unwrapDOMRef(popoverRef),
+      buttonRef: unwrappedButtonRef,


I think these changes are going to conflict with #1333

yep, I pulled the combobox and picker ones from your PR so hopefully it won't be too bad

not entirely sure why props.children is still needed in context useMemo

LFDanLu · 2020-12-08T00:14:44Z

.eslintrc.js

@@ -112,7 +112,7 @@ module.exports = {
    'space-in-parens': [ERROR, 'never'],
    'space-unary-ops': [ERROR, {words: true, nonwords: false}],
    'spaced-comment': [ERROR, 'always', {exceptions: ['*'], markers: ['/']}],
-    'max-depth': [WARN, 4],
+    'max-depth': [WARN, 5],


bumped this up since there is some deep logic in TableCollection

hmm.... i don't like bumping this up, it makes it easier to write that kind of code, how many problem spots are there? can we comment them? or can we pull out any of it to reduce the depth?

luckily there are only two instances I believe, I'll suppress the warning for them for now

…lean-up-lint-warnings

LFDanLu · 2020-12-08T00:20:46Z

packages/@react-spectrum/breadcrumbs/stories/Breadcrumbs.stories.tsx

+  {id: 3, label: 'March 2020 Assets'}
+];
+
+let DynamicBreadcrumbs = () => {


For testing the above Breadcrumb dep array changes

LFDanLu · 2020-12-08T00:23:50Z

packages/@react-stately/table/src/useTableState.ts

+  // Props.children is required here in case new rows are added/removed
+  // eslint-disable-next-line react-hooks/exhaustive-deps
  }), [props.children, props.showSelectionCheckboxes, selectionState.selectionMode]);


This one I'm kinda puzzled about since it doesn't look like context has any reliance on props.children and useCollection already has props.children in it's useMemo, but removing it broke adding rows in the CRUD example

I think @devongovett wrote the answer to this at one point in the slack channel but i can't find it. or maybe it was an old comment in a PR. Maybe he'll remember

adobe-bot · 2020-12-08T00:29:28Z

Build successful! 🎉

adobe-bot · 2020-12-08T00:34:01Z

Build successful! 🎉

adobe-bot · 2020-12-08T00:34:01Z

Build successful! 🎉

adobe-bot · 2021-06-18T00:41:23Z

Build successful! 🎉

ktabors

LGTM

# Conflicts: # packages/@react-aria/focus/src/useFocusable.tsx # packages/@react-aria/interactions/src/useInteractOutside.ts # packages/@react-spectrum/utils/src/Slots.tsx

adobe-bot · 2021-09-01T21:57:01Z

Build successful! 🎉

adobe-bot · 2021-09-21T20:44:09Z

Build successful! 🎉

reidbarber

Looks like there were 3 more to fix after with the latest update:

react-spectrum/packages/@react-spectrum/cards/src/CardBase.tsx

78:6 warning React Hook useLayoutEffect has a missing dependency: 'orientation'. Either include it or remove the dependency array react-hooks/exhaustive-deps
97:7 warning React Hook useMemo has a missing dependency: 'aspectRatioEnforce'. Either include it or remove the dependency array react-hooks/exhaustive-deps

react-spectrum/packages/@react-spectrum/dialog/src/Dialog.tsx

73:7 warning React Hook useMemo has an unnecessary dependency: 'styles'. Either exclude it or remove the dependency array. Outer scope values like 'styles' aren't valid dependencies because mutating them doesn't re-render the component react-hooks/exhaustive-deps

LFDanLu · 2021-09-21T21:24:09Z

Cleaned up the new lint warnings, thanks @reidbarber for catching those. The orientation one is handled #2362

snowystinger · 2021-09-23T16:47:56Z

packages/@react-spectrum/cards/src/CardBase.tsx

@@ -94,7 +94,7 @@ function CardBase<T extends object>(props: CardBaseProps<T>, ref: DOMRef<HTMLDiv
    actionmenu: {UNSAFE_className: classNames(styles, 'spectrum-Card-actions'), align: 'end', isQuiet: true},
    footer: {UNSAFE_className: classNames(styles, 'spectrum-Card-footer'), isHidden: isQuiet},
    divider: {UNSAFE_className: classNames(styles, 'spectrum-Card-divider'), size: 'S'}
-  }), [titleProps, contentProps, height, isQuiet, orientation]);
+  }), [titleProps, contentProps, height, isQuiet, orientation, aspectRatioEnforce]);


This one isn't quite right, aspectRatioEnforce isn't a memoized value, I'll go make a commit to address that

Ah, thanks for catching that

# Conflicts: # packages/@react-spectrum/cards/src/CardBase.tsx

adobe-bot · 2021-09-23T17:03:50Z

Build successful! 🎉

adobe-bot · 2021-09-23T17:05:11Z

Build successful! 🎉

adobe-bot · 2021-09-23T20:30:51Z

Build successful! 🎉

two failing tests will need looking into, in Table.test # Conflicts: # packages/@react-aria/interactions/src/useFocusVisible.ts # packages/@react-aria/overlays/src/useOverlayPosition.ts # packages/@react-spectrum/card/src/CardBase.tsx # packages/@react-spectrum/tabs/src/Tabs.tsx

snowystinger · 2022-10-14T17:13:17Z

closing due to #3612

Clean up a few lint warnings

fcdb525

snowystinger requested review from dannify and devongovett as code owners October 28, 2020 00:16

Merge branch 'main' into clean-up-lint-warnings

17d4031

LFDanLu reviewed Nov 4, 2020

View reviewed changes

ktabors previously approved these changes Nov 11, 2020

View reviewed changes

Merge branch 'main' into clean-up-lint-warnings

629adfe

LFDanLu added 4 commits December 4, 2020 13:41

Merge branch 'main' of https://github.com/adobe/react-spectrum into c…

fb1c9e4

…lean-up-lint-warnings

fixing some lint warnings

cb01f26

more lint fixes

7656b73

fixing new story lint

0fa07a6

LFDanLu dismissed ktabors’s stale review via 0fa07a6 December 5, 2020 01:15

snowystinger commented Dec 5, 2020

View reviewed changes

Updating useOverlayPositon to not use dep array

4829591

also changed useDomRef to use useUnwrapDOMRef so that it does not infinite loop

snowystinger commented Dec 7, 2020

View reviewed changes

LFDanLu added 2 commits December 7, 2020 15:52

fixing rest of linter warnings

e224f60

fixing table test

31d731b

not entirely sure why props.children is still needed in context useMemo

LFDanLu reviewed Dec 8, 2020

View reviewed changes

LFDanLu added 2 commits December 7, 2020 16:17

Merge branch 'main' of https://github.com/adobe/react-spectrum into c…

2430934

…lean-up-lint-warnings

removing a whiteline

afa543f

LFDanLu reviewed Dec 8, 2020

View reviewed changes

ktabors previously approved these changes Jul 14, 2021

View reviewed changes

Merge branch 'main' into clean-up-lint-warnings

2166816

# Conflicts: # packages/@react-aria/focus/src/useFocusable.tsx # packages/@react-aria/interactions/src/useInteractOutside.ts # packages/@react-spectrum/utils/src/Slots.tsx

snowystinger dismissed ktabors’s stale review via 2166816 September 1, 2021 21:49

ktabors previously approved these changes Sep 21, 2021

View reviewed changes

Merge branch 'main' into clean-up-lint-warnings

2d273d0

snowystinger dismissed ktabors’s stale review via 2d273d0 September 21, 2021 20:31

snowystinger changed the title ~~Clean up a few lint warnings~~ Clean up all lint warnings Sep 21, 2021

reidbarber reviewed Sep 21, 2021

View reviewed changes

cleaning up more lint warnings

b4fa626

Merge branch 'main' into clean-up-lint-warnings

0081d9f

snowystinger commented Sep 23, 2021

View reviewed changes

snowystinger added 2 commits September 23, 2021 11:52

fix lint warnings

0d595fa

Merge branch 'main' into clean-up-lint-warnings

ac9f2b4

# Conflicts: # packages/@react-spectrum/cards/src/CardBase.tsx

Merge branch 'main' into clean-up-lint-warnings

f21b763

LFDanLu previously approved these changes Sep 23, 2021

View reviewed changes

snowystinger dismissed LFDanLu’s stale review via ed616c4 January 4, 2022 20:31

majornista added 2 commits February 1, 2022 14:31

Merge branch 'main' into clean-up-lint-warnings

57c3437

Merge branch 'main' into clean-up-lint-warnings

98ef135

reidbarber mentioned this pull request Oct 7, 2022

Clean up lint warnings #3612

Merged

5 tasks

snowystinger closed this Oct 14, 2022

snowystinger deleted the clean-up-lint-warnings branch October 14, 2022 17:13

		let stableEmptyObject = useRef({});
		let parentSlots = useContext(SlotContext) \|\| stableEmptyObject.current;

Clean up all lint warnings #1199

Clean up all lint warnings #1199

Conversation

snowystinger commented Oct 28, 2020

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

adobe-bot commented Oct 28, 2020

adobe-bot commented Oct 28, 2020

Choose a reason for hiding this comment

snowystinger Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktabors left a comment

Choose a reason for hiding this comment

adobe-bot commented Nov 11, 2020

adobe-bot commented Dec 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Dec 8, 2020

adobe-bot commented Dec 8, 2020

adobe-bot commented Dec 8, 2020

adobe-bot commented Jun 18, 2021

ktabors left a comment

Choose a reason for hiding this comment

adobe-bot commented Sep 1, 2021

adobe-bot commented Sep 21, 2021

reidbarber left a comment

Choose a reason for hiding this comment

LFDanLu commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Sep 23, 2021

adobe-bot commented Sep 23, 2021

adobe-bot commented Sep 23, 2021

snowystinger commented Oct 14, 2022

snowystinger Nov 4, 2020 •

edited

Loading