Skip to content
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

Disable lint warnings for dependencies #1889

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

monicawheeler
Copy link
Collaborator

@monicawheeler monicawheeler commented May 16, 2024

Description

yarn lint was returning 14 warnings related to "React Hook useEffect or useCallback has a missing dependency", and "Unexpected console statement." This pull request disables linting for those warnings in order to prevent unintended side effects.

Screenshots

No visual changes.

Testing in sage-lib

  1. Pull PR locally
  2. Navigate to the fixed React files in Storybook and verify they work as expected
    a. http://localhost:4100/?path=/story/sage-drawer--default
    b. http://localhost:4100/?path=/story/sage-input--default
    c. http://localhost:4100/?path=/story/sage-panelcontrols--default
    d. http://localhost:4100/?path=/story/sage-table--default-story
    e. http://localhost:4100/?path=/story/sage-toast--default
    f. http://localhost:4100/?path=/story/sage-switch--default
    g. http://localhost:4100/?path=/story/sage-typeahead--default
    h. http://localhost:4100/?path=/docs/sage-tooltip--default

Testing in kajabi-products

  1. (LOW) Disabled linting warnings has no impact on apps consuming Sage

Related

DSS-449

Warning specifics

/sage-lib/packages/sage-react/lib/Drawer/Drawer.jsx
  43:6  warning  React Hook useEffect has a missing dependency: 'onExpandChange'. Either include it or remove the dependency array. If 'onExpandChange' changes too often, find the parent component that defines it and wrap that definition in useCallback  react-hooks/exhaustive-deps

/sage-lib/packages/sage-react/lib/Input/Input.jsx
  106:6  warning  React Hook useEffect has a missing dependency: 'inputStyles'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

/sage-lib/packages/sage-react/lib/Input/Input.story.jsx
  154:5  warning  Unexpected console statement  no-console

/sage-lib/packages/sage-react/lib/PanelControls/PanelControls.story.jsx
  150:6  warning  React Hook useEffect has a missing dependency: 'fetchData'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

/sage-lib/packages/sage-react/lib/Table/Table.jsx
  176:6  warning  React Hook useEffect has a missing dependency: 'buildHeaders'. Either include it or remove the dependency array      react-hooks/exhaustive-deps
  182:6  warning  React Hook useEffect has a missing dependency: 'getSelectionType'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

/sage-lib/packages/sage-react/lib/Table/TableRow.jsx
  79:6  warning  React Hook useEffect has a missing dependency: 'hasBorders'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

/sage-lib/packages/sage-react/lib/Toast/Toast.jsx
  45:6  warning  React Hook useEffect has a missing dependency: 'dismiss'. Either include it or remove the dependency array  react-hooks/exhaustive-deps
  50:6  warning  React Hook useEffect has a missing dependency: 'dismiss'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

/sage-lib/packages/sage-react/lib/Toggle/Switch.story.jsx
  106:37  warning  Using propTypes from another component is not safe because they may be removed in production builds  react/forbid-foreign-prop-types

/sage-lib/packages/sage-react/lib/Tooltip/TooltipElement.jsx
  68:6  warning  React Hook useLayoutEffect has missing dependencies: 'parentDomRect' and 'position'. Either include them or remove the dependency array. Mutable values like 'tooltipRef.current' aren't valid dependencies because mutating them doesn't re-render the component  react-hooks/exhaustive-deps

/sage-lib/packages/sage-react/lib/Typeahead/Typeahead.jsx
  42:38  warning  React Hook useEffect has a missing dependency: 'filterList'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

/sage-lib/packages/sage-react/lib/common/hooks/useDebounceEffect.jsx
  4:20  warning  React Hook useCallback has a missing dependency: 'effect'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

/sage-lib/packages/sage-react/lib/common/hooks/useFocusTrap.jsx
  65:6  warning  React Hook useEffect has missing dependencies: 'containerRef', 'onDeactivateFn', and 'returnFocus'. Either include them or remove the dependency array. Mutable values like 'containerRef.current' aren't valid dependencies because mutating them doesn't re-render the component  react-hooks/exhaustive-deps

✖ 14 problems (0 errors, 14 warnings)

@monicawheeler monicawheeler self-assigned this May 16, 2024
@monicawheeler monicawheeler requested a review from a team May 16, 2024 19:38
@@ -103,7 +103,7 @@ export const Input = React.forwardRef(({
if (affixUpdatesNeeded) {
updateStyles(newInputStyles);
}
}, [prefix, suffix]);
}, [prefix, suffix, inputStyles]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful with these types of things...they can sometimes cause unexpected side effects. I would suggest you test these manually.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ju-Skinner - I did manually test these in the local dev environment for the doc site, and all the functionality and styles worked as expected. Anything in particular you suggest I look out for?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consider disabling these instead of trying to add the dependency. This could have an adverse side-effect that we may not catch until it's in prod.

@monicawheeler monicawheeler requested a review from a team May 16, 2024 19:42
@QuintonJason QuintonJason self-requested a review May 16, 2024 23:19
Copy link
Contributor

@QuintonJason QuintonJason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-05-16 at 6 20 21 PM

Getting the following warning in the browser console

@monicawheeler
Copy link
Collaborator Author

Screenshot 2024-05-16 at 6 20 21 PM

Getting the following warning in the browser console

These are different from what I sought to resolve. These errors occur on the production side as well. What are your thoughts of dealing with those in another task?

@monicawheeler monicawheeler requested review from QuintonJason and a team May 17, 2024 17:01
@monicawheeler monicawheeler marked this pull request as draft May 28, 2024 19:34
@monicawheeler monicawheeler force-pushed the fix/hook-useEffect-dependency-warnings branch from afe1e93 to d2300e8 Compare June 17, 2024 14:57
@monicawheeler monicawheeler requested review from QuintonJason and a team and removed request for a team and QuintonJason June 17, 2024 15:01
@monicawheeler monicawheeler marked this pull request as ready for review June 17, 2024 15:39
@monicawheeler monicawheeler changed the title Fix warnings for React files Disable lint warnings for dependencies Jun 17, 2024
Copy link
Contributor

@QuintonJason QuintonJason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and console is error-free

@monicawheeler monicawheeler requested a review from a team June 17, 2024 16:24
@monicawheeler monicawheeler merged commit d28a6eb into develop Jun 17, 2024
6 checks passed
@monicawheeler monicawheeler deleted the fix/hook-useEffect-dependency-warnings branch June 17, 2024 18:05
@pixelflips pixelflips mentioned this pull request Jun 21, 2024
ju-Skinner pushed a commit that referenced this pull request Jun 21, 2024
* chore: update Storybook source path for local and prod environment

* Disable lint warnings for dependencies (#1889)

* chore: update Storybook source path for local and prod environment

* fix: resolve linting errors

* fix: revert preview-head file change

---------

Co-authored-by: Julian Skinner <[email protected]>

* fix(label): convert icon to pine (#1896)

* fix(Icon): adds back rest to icon component (#1898)

* fix: adds back rest to icon

* fix: add rest to element

---------

Co-authored-by: Julian Skinner <[email protected]>
Co-authored-by: Monica Wheeler <[email protected]>
This was referenced Jun 21, 2024
ju-Skinner pushed a commit that referenced this pull request Jun 21, 2024
* chore: update Storybook source path for local and prod environment

* fix: resolve linting errors

* fix: revert preview-head file change

---------

Co-authored-by: Julian Skinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants