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

Component with forwardRef passed as prop #613

Open
tu4mo opened this issue Aug 12, 2021 · 11 comments
Open

Component with forwardRef passed as prop #613

tu4mo opened this issue Aug 12, 2021 · 11 comments

Comments

@tu4mo
Copy link

tu4mo commented Aug 12, 2021

const A = () => <div />
const B = React.forwardRef(() => <div />)

What I get:

<Comp
  a={A}
  b={{
    $$typeof: Symbol(react.forward_ref),
    render: _c3
  }}
/>

What I expect:

<Comp
  a={A}
  b={B}
/>

Repro: https://codesandbox.io/s/dank-cdn-ftvik?file=/src/App.js

@armandabric
Copy link
Collaborator

armandabric commented Sep 28, 2021

Thanks to #617 we are now supporting React.forwardRef(). But I still reproduce your issue with this test:

it('should use inferred function name as display name for `forwardRef` element when used in a props', () => {
    const Comp = () => <div />;
    const A = () => <div />;
    const B = React.forwardRef(function B(_props, ref) {
      return <div ref={ref} />;
    });

    expect(reactElementToJSXString(<Comp a={A} b={B}></Comp>)).toEqual(
      `<Comp
  a={function noRefCheck() {}}
  // Should be <B />
  b={{
    $$typeof: Symbol(react.forward_ref),
    render: function noRefCheck() {}
  }}
/>`
    );
  });

As it's fresh in it's mind maybe @Andarist have an idea?

@Andarist
Copy link
Contributor

It's an interesting case - this is not a valid React element so it doesn't go into this logic:

if (isValidElement(propValue)) {
return `{${formatTreeNode(
parseReactElement(propValue, options),
true,
lvl,
options
)}}`;
}

A valid React element is a result of the React.createElement call (so basically the output of JSX: b={<B/>}). This is a component type~, like a class, function, result of the React.memo and more.

@armandabric
Copy link
Collaborator

armandabric commented Sep 30, 2021

Good point. I'm not sure what we could propose as in fact B is only an object parsed to a prop: not a react element.

@Andarist
Copy link
Contributor

Well, it could still be formatted somehow - since we can recognize that it's not "just an object". But I don't believe printing <Comp/> is OK here. The suggested expected formatting could work OK here - printing Comp in this case. Note that in the given example we would not be able to print B because everything is anonymous there, but with inferred names and display names something nicer could be printed.

I probably won't take this work on myself though - it shouldn't be overly hard though with the groundwork being laid out already.

@armandabric
Copy link
Collaborator

I agree with you we could easily add a dedicated render for this use case but I lack of idea on the shape of this render.

If people really need this they could explain us their use case and make us proposal. If it's a reasonable use case we will consider to handle this. I keep this issue open for the discussion.

@tu4mo
Copy link
Author

tu4mo commented Sep 30, 2021

My exact use case is documenting polymorphic components with as prop.

<Link as={RouterLink}>Link</Link>

If the as component is wrapped with a forwardRef (like they usually are) it does not work.

@armandabric
Copy link
Collaborator

I see. We could maybe have a dedicated behavior for the as props. It's a common usage

@Andarist
Copy link
Contributor

Andarist commented Oct 7, 2021

OTOH - this really could be handled in the same way for any prop. People probably rarely want to see the react element's internal structure in their stringified trees

@iMoses-Apiiro
Copy link

iMoses-Apiiro commented Dec 24, 2021

I think we have two cases here to handle:

  • a prop which is of type object and has a $$typeof property of type symbol, in which we can recursively search for a displayName
  • a prop which is of type function that has a displayName, or fallback to name

The first case can be solved like this:

// formatPropValue.js.flow:43

  if (typeof propValue.$$typeof === 'symbol') {
    return `{${parseReactElement(createElement(propValue), options).displayName}}`;
  }

Or we can export getReactElementDisplayName so we can bypass parseReactElement and get the displayName directly

Regarding function components, what about this method? Could also improve regular functions display by using the name as fallback before noRefCheck

// formatFunction.js.flow:16

const getFucntionDisplayName = (fn: Function): string | undefined =>
  fn.displayName || fn.name;

export default (fn: Function, options: Options): string => {
  const {
    showFunctions,
    functionValue = defaultFunctionValue,
    displayName: displayNameFn = getFucntionDisplayName,
  } = options;
  if (!showFunctions && functionValue === defaultFunctionValue) {
    return displayNameFn(fn) || defaultFunctionValue(noRefCheck);
  }

  return functionValue(fn);
};

@armandabric @Andarist WDYT?

@as-zlynn-philipps
Copy link

Is there a PR for this? I don't see one linked.

@as-zlynn-philipps
Copy link

as-zlynn-philipps commented Mar 4, 2022

OTOH - this really could be handled in the same way for any prop. People probably rarely want to see the react element's internal structure in their stringified trees

Agreed. function noRefCheck() {} isn't useful either. Nor is () => {} when it should be the element type (Comp).

image

Here, I'd expect onClick to be () => {} but maybe that's a Storybook actions issue.


EDIT: Okay, so I figured out how to configure the options for this package in Storybook via global parameters.jsx.

export const parameters = {
  jsx: {
    showFunctions: true,
    // ...
  }
}

I can set showFunctions to true there, but then I run into the problem that this issue #613 is really about. Instead of the component name, it's gobbledegook. And, since in my case, the function name is a bit different from the exported module name, that adds a layer of complexity. In my case, I might be able to leverage functionValue to achieve the desired result.

The reason onClick was showing function noRefCheck() {} is because my story uses Controls, so I'm setting the icon prop dynamically in the UI. Since sortProps is true by default, it looks like the fallback value for icon was being assigned to onClick I think? Maybe? Due to this bit in the README?

options.sortProps: boolean, default true
Either to sort or not props. If you use this lib to make some isomorphic rendering you should set it to false, otherwise this would lead to react invalid checksums as the prop order is part of react isomorphic checksum algorithm.

Either way, explicitly setting sortProps to false yields a more expected result:

image

Just reporting my findings. ✌️


EDIT 2:

With the following config:

BasicUsage.parameters = {
  jsx: {
    filterProps: ['onClick'],
    functionValue: (fn: any) => fn.name.replace('Svg', ''),
    showFunctions: true,
  },
}

Voila:

image

Although, it's kinda jank because that functionValue would apply to any function prop. It would be neat if the prop name were passed as well, that way you could modify functionValue conditionally based on the component prop. Even better, a propValue function that takes in the prop key/value and allows you to customize the output. Related: #278, #530, #682

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

No branches or pull requests

5 participants