False positive for function-component-definition when function returns null

[UncleClapton]

I hava a function which react/function-component-definition is falsely reporting as a function component. This only seems to occur when null is returned at the end of a function.

My rule config is:

'react/function-component-definition': ['error', {
  namedComponents: 'function-declaration',
  unnamedComponents: 'arrow-function',
}],

function that triggers a false positive:

const selectAvatarByUserId = (state, id) => {
  const user = selectUserById(state, id)

  if (user) {
    return user.attributes.image || `//api.adorable.io/avatars/${user.id}`
  }

  return null
}

Removing return null or returning any other value fixes the error. I was able to get around this by reversing my if statement, but I'm still reporting this as a potential bug.

[ljharb]

In this case, the component detection indeed should not treat this as a component because all of the following apply:

1. has no jsx in it
2. taking 3+ arguments in general would disqualify it, but in this case taking 2 arguments, but not having any contextTypes property attached to it which is required for the second argument to be useful

[ljharb]

cc @alexzherdev for component detection

[joaopslins]

Had the same issue with a generator (while using sagas):

export function getLoadMoreSaga(schema) {
  function* saga(action) {
    const { key, url } = action.payload;
    if (!isNil(url)) {
      return yield getSaga(key, [schema], url, action.type, true);
    }
    return null;
  }

  return saga;
}

Inverting the if conditional solved the issue, but I think this shouldn't be triggered by the rule

[osmestad]

We are also hitting issues with this as the auto-fix breaks the code when "fixing" generators into arrow functions. I tried having a look in the Components detection code (lib/util/Components.js), but could not find any tests for it? or any way of testing the detect function it exposes directly?

[ljharb]

@osmestad it's not tested directly, only by virtue of rules.

A PR with failing test cases would be an ideal minimum, if also including the fix wasn't an option.

[osmestad]

I might be able to try next week, but the root cause seems to be the heuristic that assumes a return null; means it is a React Component :-)

[ljharb]

In general, that it's a generator should automatically invalidate it as being a component - that's probably the easiest fix here.

[osmestad]

True, generators should be easy to fix, and I guess they are the worst part, as that breaks the code. But as in the original report there are also other functions that are not React related that get transformed, here are some examples from our code-base (which uses Flow):

export function getItemIdAtIndex(mediaSession: Media, index: number): ?number {
  if (Array.isArray(mediaSession.items) && mediaSession.items[index]) {
    return mediaSession.items[index].itemId;
  }

  return null;
}

function ensureValidSourceType(sourceType: string) {
  switch (sourceType) {
    case 'ALBUM':
    case 'PLAYLIST':
    case 'MIX':
    case 'ARTIST':
    case 'SEARCH':
    case 'MY_TRACKS':
    case 'MY_VIDEOS':
      return sourceType;
    default:
      return null;
  }
}

export function actionToStartReason(action: StartActions): ?PlayLogStartReason {
  switch (action.type) {
    case playQueueActions.ADD_NOW:
      return 'EXPLICIT_PLAY';
    default:
      return null;
  }
}

[ljharb]

I think it’s also helpful to try to consider that functions that are named starting with a lowercase letter, aren’t components.