Eslint: extend plugin:react-hooks/recommended

[dominikdosoudil]

The only problem is that there are few internal functions that have prefix "use" and eslint thinks that they are hooks.

Should the code be refactored along with this PR or it is just ok that some false positive errors would pop up with accepting this PR?

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
semantic-ui-react	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 13, 2024 8:24am

[layershifter]

Should the code be refactored along with this PR or it is just ok that some false positive errors would pop up with accepting this PR?

@dominikdosoudil let's create a separate PR to rename these functions. I believe the problem is with classNameBuilders.js.

I suggest:

• Update src/lib/index.js to do (so we don't need to do all renames in a single PR):

  export {
    useKeyOnly,
  + useKeyOnly as getKeyOnly,
    useKeyOrValueAndKey,
  + useKeyOrValueAndKey as getKeyOrValueAndKey,
    // etc.
  } from './classNameBuilders'

• Create a set of PRs to replace useKeyOnly with getKeyOnly, useKeyOrValueAndKey ➡️ getKeyOrValueAndKey, etc. Keep each PR with some reasonable amount of files 🐱
• Rename functions i.e. useKeyOnly to getKeyOnly
• Enable the ESLint plugin 🎉

[dominikdosoudil]

• Rename functions i.e. useKeyOnly to getKeyOnly

@layershifter That sounds good, however this step will still affect a lot of files as the useKeyOnly as getKeyOnly, will need to be changed to getKeyOnly,.

If that's ok with you, I'll follow the steps and mention the individual PRs here.

[layershifter]

@layershifter That sounds good, however this step will still affect a lot of files as the useKeyOnly as getKeyOnly, will need to be changed to getKeyOnly,.

Indeed, but if PRs will be sliced by files' location i.e. all (use* => get*) in src/modules, then src/elements, etc. It will be less files at once :)

[dominikdosoudil]

Can I suggest another approach which would avoid the last big bang?

We might create aliases for useKeyOnly and deprecate the original first:

/**
+ * @deprecated
 * Props where only the prop key is used in the className.
 * @param {*} val A props value
 * @param {string} key A props key
 *
 * @example
 * <Label tag />
 * <div class="ui tag label"></div>
 */
export const useKeyOnly = (val, key) => val && key

+ export const getKeyOnly = useKeyOnly;

Step 2 - incrementally replace the function name in individual files to make PR less big:

export {
-  useKeyOnly,
+ getKeyOnly,
-  useKeyOrValueAndKey,
+ getKeyOrValueAndKey,
  // etc.
} from './classNameBuilders'

Step 3 - remove the original functions and inline the aliases:

/**
- * @deprecated
 * Props where only the prop key is used in the className.
 * @param {*} val A props value
 * @param {string} key A props key
 *
 * @example
 * <Label tag />
 * <div class="ui tag label"></div>
 */
- export const useKeyOnly = (val, key) => val && key
-
- export const getKeyOnly = useKeyOnly;
+ export const getKeyOnly = (val, key) => val && key

This way, there should not be the large number of changes in imports after finally renaming the functions in their declarations.

[layershifter]

@dominikdosoudil yeah, that's actually was my proposal 🐱 Sorry if I was unclear :)

^ that would avoid deprecations with the same result, but your proposal is even better :)

[dominikdosoudil]

Step 1: #4500 aliases

[layershifter]

Step 1: #4500 aliases

@dominikdosoudil it's merged 😉

[dominikdosoudil]

After merging #4505, there will be few warnings and one remaining error:

React Hook "React.useEffect" is called conditionally. React Hooks must be called in the exact same order in every component render

caused by

  if (process.env.NODE_ENV !== 'production') {
    React.useEffect(() => {
      ...
    }, [exclusive, activeIndex])
  }

I think that it might be ignored because we are smarter than eslint here and we know that NODE_ENV won't probably change during runtime.