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

Fix joinClasses’s flowtype function call arity #338

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acusti
Copy link
Contributor

@acusti acusti commented Feb 19, 2019

The current flow type, which comes out to (className: mixed) => string, is incorrect (the function has no maximum arity because it iterates over arguments), and causes the following flow error in the master branch of facebook/draft-js:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/component/contents/DraftEditorContents-core.react.js:202:11

Cannot call joinClasses because no more than 1 argument is expected by function [1].

     src/component/contents/DraftEditorContents-core.react.js
     197│           lastWrapperTemplate !== wrapperTemplate ||
     198│           currentDepth === null ||
     199│           depth > currentDepth;
     200│         className = joinClasses(
     201│           className,
     202│           getListItemClasses(blockType, depth, shouldResetCount, direction),
     203│         );
     204│       }
     205│
     206│       const Component = CustomComponent || DraftEditorBlock;

     node_modules/fbjs/lib/joinClasses.js.flow
 [1]  16│ function joinClasses(className: mixed): string {



Found 1 error

This PR preserves the function’s logic entirely and resolves the issue by casting the type to (...classNames: Array<mixed>) => string, which is correct.


As background, this error comes from a change to flow rolled out back in v0.47.0 and described here. In that blog post, the author recommends:

This will, unfortunately, cause Flow to complain about stubs and variadic functions which are written using arguments. However, you can fix these by using rest parameters

So, if using argument rest parameters is allowed, another fix would be:

function joinClasses(className: mixed, ...otherClasses: Array<mixed>): string {
  let newClassName = ((className: any): string) || '';
  const extraArgsLength = otherClasses.length;

  if (extraArgsLength > 0) {
    for (let index = 0; index < extraArgsLength; index++) {
      const nextClass = otherClasses[index];

      if (nextClass) {
        newClassName = (newClassName ? newClassName + ' ' : '') + nextClass;
      }
    }
  }

  return newClassName;
}

module.exports = joinClasses;

or just:

function joinClasses(...classNames: Array<mixed>): string {
  return classNames.reduce((str, className) => {
    let nextClass = ((className: any): string) || '';

    return nextClass ? (str ? str + ' ' : '') + nextClass : str;
  }, '');
}

module.exports = joinClasses;

I’d be happy to PR either of those alternatives if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants