FortAwesome / react-fontawesome

Font Awesome React component
https://fontawesome.com
MIT License
3.67k stars 262 forks source link

Add a default to className #559

Closed ABuffSeagull closed 2 months ago

ABuffSeagull commented 3 months ago

I was getting errors with className.split in my project.

Update: I realized this is cause I'm using the new React v19, which silently ignores prop-types.

robmadole commented 3 months ago

I think I have this fixed in 0.2.1. Closing.

WyvernDrexx commented 3 months ago

@robmadole this is not fixed in version 0.2.1 please check. I am using React 18

The issue seems to be this, if the props has { className: undefined } as a result of passing through props of multiple components then, const allProps = { ...defaultProps, ...props } this would not have any affect on the props. Hence, the className.split()would throw error.

https://playcode.io/1874401

WyvernDrexx commented 3 months ago

Will something like this work:

const allProps = {}

  //We clean the undefined prop to use default prop values if present
  for (const propName in props) {
    const propValue = props[propName]
    if (typeof propValue === 'undefined') {
      allProps[propName] = defaultProps[propName]
    } else {
      allProps[propName] = propValue
    }
  }
robmadole commented 3 months ago

Oh shoot. Yeah I get why that's happening @WyvernDrexx. We could probably do your trick or we could make a specific exception for just the className. Let me think on this and see if I can get another release out today.

WyvernDrexx commented 3 months ago

@robmadole Much better approach would be this..

  const allProps = { ...props }

  /**
   * Loop through default props and replace any undefined user prop value with the value from defaultProps.
   */
  for (const prop in defaultProps) {
    const defaultValue = prop[defaultProps]
    if (typeof allProps[prop] === 'undefined') allProps[prop] = defaultValue
    else allProps[prop] = props[prop]
  }

This code will also clean all the undefined default props value and allProps will also have all defaultProps.

jonny-dungeons commented 3 months ago

Is there anything we are waiting on for this to be merged? Are there any reviewers available for this?

This is blocking my team and I on a project and unfortunately we have made the upgrade to 0.2.1.

shehi commented 2 months ago

Same here. This should be fixed, especially when TS typing lists className prop as skippable:

image

robmadole commented 2 months ago

Just released 0.2.2. Let's see if that addresses things, folks. Let me know.