Dropdown with inline JS issue in 3.9.4

[Tchekda]

Description

After upgrading to v3.9.4, I faced a major problem with my dropdown.

Steps to Reproduce

1. Update to v3.9.4
2. Create a Dropdown looking like this one :
   (You can obviously use any bool condition instead of hasRole)

<Dropdown
  id="security-dropdown"
  trigger={
    <a href="#!">
      Sécurité <Icon right>arrow_drop_down</Icon>
    </a>
  }>
  {hasRole('ROLE_GUARDIAN') ? (
    <>
      <Link to={`/${userType}/email`}>Adresse Mail</Link>
      <Link to={`/${userType}/other`}>Autre</Link>
    </>
  ) : (
    <Link to={`/${userType}/password`}>Mot de Passe</Link>
  )}
</Dropdown>

3. Admire this beautiful error : TypeError: (intermediate value).filter is not a function located at renderItems xxxxx/node_modules/react-materialize/lib/Dropdown.js:54

Expected behavior: Display correctly the dropdown as it was working in v3.9.3

Actual behavior: The temporary fix that I found is to make to separate conditions.

<Dropdown
  id="security-dropdown"
  trigger={
    <a href="#!">
      Sécurité <Icon right>arrow_drop_down</Icon>
    </a>
  }>
  {hasRole('ROLE_GUARDIAN') && (
    <Link to={`/${userType}/email`}>Adresse Mail</Link>
  )}
  {hasRole('ROLE_GUARDIAN') && (
    <Link to={`/${userType}/other`}>Autre</Link>
  )}
  {!hasRole('ROLE_GUARDIAN') && (
    <Link to={`/${userType}/password`}>Mot de Passe</Link>
  )}
</Dropdown>

Explanations : The PR #1164 made by @MauAbata changed the way Dropdowns are rendered, that had impact on my code, and I suppose and many others.

Versions

react-materialize: 3.9.4
materialize-css: 1.0.0
react: 17.0.1

[MauAbata]

I suspect this has to do with how the fragment is passed to the children prop in the first part of your conditional. Did previous versions of react-materialize properly render the links within that fragment into their own <li/> elements?

I'll reproduce this locally and see if I can get a fix for that previous PR.

[Tchekda]

Thank you @MauAbata for your quick answer, in 3.9.3, both link where in the same <li> :

and were correctly rendered :
.

You are right about something, both link should have their own <li> and not be in the same one.. But it still worked in 3.9.3... 🤷‍♂️

[alextrastero]

Yes, I think this is specific to how the fragment works, how about doing smth like this:

const links = hasRole('ROLE_GUARDIAN')
    ? [
      <Link key="adresse" to={`/${userType}/email`}>Adresse Mail</Link>,
      <Link key="other" to={`/${userType}/other`}>Autre</Link>
    ]
    : [
      <Link key="pass" to={`/${userType}/password`}>Mot de Passe</Link>,
    ];

  return (
    <Dropdown
      id="security-dropdown"
      trigger={
        <a href="#!">
          Sécurité <Icon right>arrow_drop_down</Icon>
        </a>
      }>
      {links}
    </Dropdown>
  );

[MauAbata]

I just checked the docs, and fragments are indeed passed as a single element. In this case, props.children is not an array, and my assumption that it would be was exceptionally naive.

I'm testing another variant of my PR that simply passes on null values yielded in Children.map, which seems far more proper than assuming props.children is an array. I had forgotten that children can be a single element, which is why Children.map is even a thing, eh?

[Tchekda]

Yes, I think this is specific to how the fragment works, how about doing smth like this:

const links = hasRole('ROLE_GUARDIAN')
    ? [
      <Link key="adresse" to={`/${userType}/email`}>Adresse Mail</Link>,
      <Link key="other" to={`/${userType}/other`}>Autre</Link>
    ]
    : [
      <Link key="pass" to={`/${userType}/password`}>Mot de Passe</Link>,
    ];

  return (
    <Dropdown
      id="security-dropdown"
      trigger={
        <a href="#!">
          Sécurité <Icon right>arrow_drop_down</Icon>
        </a>
      }>
      {links}
    </Dropdown>
  );

Hi, thank you so much for this solution, it seems to work perfectly now, each link has it's own <li> !

Do you expect to fix this, or users will have to use arrays to do similar things ?

[alextrastero]

@Tchekda could this be an edge case worth fixing? I will certainly keep track of this and if it's reported again we'll need to revisit this. Thanks @MauAbata and @Tchekda

[Tchekda]

I do agree about the fact that it is an edge case, that's why I asked if an update was on it's way or not, so I could close the issue.

Thank you both !

[MauAbata]

@alextrastero I just realized that the current code will most likely break on any single-element Dropdown, which includes simple things like

<Dropdown>
  <a>I'm an only child</a>
</Dropdown>

Since the components prop in that case is just the one child, not an array. I've created a PR to fix this and recurse into Fragments: #1168

@Tchekda Out of curiosity, and if it is easily possible for you to build from the branch, can you try my current PR with your original code using the fragments?

[alextrastero]

Ouch, how come the specs didn't catch that?

[Tchekda]

@Tchekda Out of curiosity, and if it is easily possible for you to build from the branch, can you try my current PR with your original code using the fragments?
Still same error with your PR.....

[MauAbata]

@Tchekda You mean, still the same .filter is not a function error? Because that code has been removed in the PR. Or is it something else now?

@alextrastero There aren't any specs that render it with a single element for children, it seems. The specs render it with no elements or an array of elements, and in the no element case the || [] condition I added (specifically to fix the tests) caught that and coerced children into an empty array instead.

I am, for peace of mind, working on adding some snapshots to the tests to handle the cases I mentioned in my PR.

**Late update: ** I've added tests that should address a few cases. The snapshots in the PR also represent what the component is now rendering in those cases, which should cover the edge cases addressed here. If you can think of other edge cases I'll add them to the test suite to be sure, while I have the code open.

[Tchekda]

@Tchekda You mean, still the same .filter is not a function error? Because that code has been removed in the PR. Or is it something else now?

I'm dumb sorry, I've cloned your repo but forgot to checkout your fix branch 🤦‍♂️ !
It's working now, each Link inside of the Fragment is getting it's own <li> !

Thank you so much, if you need something more, feel free to ask !!

[alextrastero]

Should be fixed in 3.9.5