Semantic-Org / Semantic-UI-React

The official Semantic-UI-React integration
https://react.semantic-ui.com
MIT License
13.21k stars 4.05k forks source link

FormInput: Support for defaultProps will be removed [v3] #4426

Closed DroopeGu1 closed 9 months ago

DroopeGu1 commented 1 year ago

Hello, I am using the library in version "semantic-ui-css": "^2.5.0", and "semantic-ui-react": "^2.1.4", along with react 18 and next 13 , and I am having the following warning or error, I searched but found no solution, would not be compatible with the version I use?

image

welcome[bot] commented 1 year ago

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you've completed all the fields in the issue template so we can best help.

We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

layershifter commented 1 year ago

FYI It's not only about FormInput, there is a lot of components that use defaultProps, for example:

https://github.com/Semantic-Org/Semantic-UI-React/blob/7d3e162fa055f3851683c8cae0b1d282e62ee10d/src/modules/Search/Search.js#L682-L688

https://github.com/Semantic-Org/Semantic-UI-React/blob/7d3e162fa055f3851683c8cae0b1d282e62ee10d/src/addons/Radio/Radio.js#L36-L38

https://github.com/Semantic-Org/Semantic-UI-React/blob/7d3e162fa055f3851683c8cae0b1d282e62ee10d/src/views/Feed/FeedUser.js#L38-L40

https://github.com/Semantic-Org/Semantic-UI-React/blob/7d3e162fa055f3851683c8cae0b1d282e62ee10d/src/views/Feed/FeedLike.js#L34-L36

https://github.com/Semantic-Org/Semantic-UI-React/blob/7d3e162fa055f3851683c8cae0b1d282e62ee10d/src/elements/Icon/IconGroup.js#L43-L45

Actions

rsodre commented 1 year ago

Anyone know of a way to suppress that specific warning or some react version to rollback?

Or anyone working on a PR? If not, I'm almost taking it.

This is soooo annoying! My work feels like that episode in Seinfeld where Kramer had a big red neon sign by his window.

layershifter commented 1 year ago

Or anyone working on a PR? If not, I'm almost taking it.

Nope, this issue waits for the hero 🦸

rsodre commented 1 year ago

I'll try to find some time to do it. Will reply here again if I start.

Meanwhile... https://stackoverflow.com/a/64197014/360930

achimkoellner commented 1 year ago

We experience the warning with these components

Table
Table.Header
Table.Body
Table.Footer
Table.Row
Table.Cell
Table.HeaderCell
joequery commented 1 year ago

@layershifter I've attempted to start work on this. On my very first attempt at updating MessageItem, removing the defaultProps and changing the function to look like

  const MessageItem = React.forwardRef(function (props, ref) {
    const { children, className, content, as="li" } = props

did not seem to actually set the as value. The unittests failed with

FAILED TESTS:
  MessageItem
    ✖ renders an li tag
      Chrome Headless 97.0.4691.0 (Linux x86_64)
      AssertionError: expected <div /> to have a 'li' tag name, but it has 'div'
           HTML:
           <div className="content" />
          at Context.<anonymous> (test/tests.bundle.js:540928:97)

    forwardsRef
      ✖ forwards ref to "li"
        Chrome Headless 97.0.4691.0 (Linux x86_64)
        AssertionError: expected spy to have been called with arguments matching { tagName: "LI" }
        <div class="content"></div> { tagName: "LI" }
            at Context.<anonymous> (test/tests.bundle.js:541688:28)

Any advice here would be appreciated.

joequery commented 1 year ago

So, this does work. Is this method acceptable?

~ const MessageItem = React.forwardRef(function (passedProps, ref) {
+   const defaultProps = { as: 'li' }
+   const props = { ...defaultProps, ...passedProps }
    const { children, className, content } = props
joequery commented 1 year ago

Noting here that removal of defaultProps is going to potentially affect this portion of the shorthand props common test grouping:

    if (alwaysPresent || (Component.defaultProps && Component.defaultProps[propKey])) {
      it(`has default ${name} when not defined`, () => {
        wrapper = mount(React.createElement(Component, requiredProps))

        wrapper.should.have.descendants(ShorthandComponent)
      })
    } 

(Link to code in test/specs/commonTests/implementsShorthandProp.js)

Since these testcases are dynamically generated, it's going to be important that, if reasonable, the number of tests asserted is the same before and after these changes. Failing to generate a testcase would result in a silent failure.

layershifter commented 1 year ago

@layershifter I've attempted to start work on this. On my very first attempt at updating MessageItem, removing the defaultProps and changing the function to look like

@joequery thanks for looking to it and sorry for delay in the reply.

My proposal is to replace getElementType with getComponentType that things better:

// src/collections/Message/MessageItem.js
-  const ElementType = getElementType(MessageItem, props)
-  const ElementType = getComponentType(props, { defaultAs: 'li' })
// src/lib/index.js
+export getComponentType from './getComponentType'
export getElementType from './getElementType'
// src/lib/getComponentType.js
/**
 * Returns a createElement() type based on the props of the Component.
 * Useful for calculating what type a component should render as.
 *
 * @param {object} props A ReactElement props object
 * @param {object} [options={}]
 * @param {function} [options.defaultAs] A default element type.
 * @param {function} [options.getDefault] A function that returns a default element type.
 * @returns {string|function} A ReactElement type
 */
function getComponentType(props, options = {}) {
  const { defaultAs, getDefault } = options

  // ----------------------------------------
  // user defined "as" element type

  if (props.as && props.as !== defaultAs) return props.as

  // ----------------------------------------
  // computed default element type

  if (getDefault) {
    const computedDefault = getDefault()
    if (computedDefault) return computedDefault
  }

  // ----------------------------------------
  // infer anchor links

  if (props.href) return 'a'

  // ----------------------------------------
  // use defaultProp or 'div'

  return defaultAs || 'div'
}

export default getComponentType

in the end, getElementType will be removed. Does it make sense?

layershifter commented 9 months ago

Released in 3.0.0-beta.2 🎉

Edanmer commented 4 months ago

SideBar component still gives the defaultProps message on 3.0.0-beta.2

EventListener2: Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead