Semantic-ui-react: Prepare for upcoming react deprecations introduced in 16.3

[justinobney]

Suggestion: begin migration to new react lifecycles while retaining backwards compatibility via react-lifecycles-compat

Example: https://www.youtube.com/watch?v=G9S1IghlkCI

Steps

• Use new lifecycle methods introduced in react 16.3 in a component that renders react-semantic-ui components.

Expected Result

• React does not log errors/warning about upcoming deprecations to the console.

Actual Result

• React IS logging errors/warning about upcoming deprecations to the console.

Version

0.79.1

Testcase

[welcome[bot]]

👋 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.

[levithomason]

@justinobney this is great, could you start a PR for this?

[joshuaobrien]

Is this up for grabs? I'm happy to do it.

[justinobney]

@joshuaobrien that would be great. I doubt I could get to it for a few weeks.

[joshuaobrien]

I'm on it. Will get started on the weekend :)

[msrikanth508]

I would like to start on this, Couple of life cycle methods are marked as UNSAFE in 16.3 and two new methods getDerivedStateFromProps, getSnapshotBeforeUpdate got introduced. @levithomason what exactly do we need to do?

[joshuaobrien]

Hi @msrikanth508, I am working on this at the moment. What we need to do is refactor the components that use the deprecated methods so that they no longer use them. Functionality that was in these deprecated lifecycle methods needs to be moved into other lifecycle methods.

My experiences so far is that these changes are not always clean cut. Also, changes to tests are likely required to accommodate the new methods.

[gavinmeier25]

Is this still needed I would be happy to hop on it this upcoming weekend and take a gander?

[layershifter]

Yep, feel free to pickup 👍

[tri-hoang]

I would love to work on this if it's still needed?

[joshuaobrien]

It's still needed. I haven't got around to it. There is a PR here: https://github.com/Semantic-Org/Semantic-UI-React/pull/2757

[kaiz123]

Hey is this still open to work on @justinobney @joshuaobrien

[kohakukun]

In case nobody has addressed this, I created a PR https://github.com/Semantic-Org/Semantic-UI-React/pull/3244 .

[Stefie]

Hi,
I found out today that the use of the old lifecycle methods might be the cause for some of the open bugs, I'm sure that it's the case for at least the one I referenced above.

To be more precise: If people are using componentDidMount and componentDidReceiveProps in their projects as recommended together with SUI components that still use componentWillMount and componentWillReceiveProps to handle their internal component state (like Dropdown), the internal component state of the SUI components are not getting updated as expected.

Unfortunately I didn't have the time today to fix that in the Dropdown component today, so I'm just putting this here as a note to bump up the issue

[layershifter]

componentWillMount()

• [ ] src/modules/Dropdown/Dropdown.js - can be replaced with constructor/didMount()
• [x] src/modules/Search/Search.js - can be replaced with constructor/didMount()

componentWillUpdate()

• [ ] docs/src/components/CarbonAd/CarbonAdNative.js - replace with didMount()

componentWillReceiveProps()

Components

• [x] src/addons/TransitionablePortal/TransitionablePortal.js replace with getDerivedStateFromProps()
• [x] src/behaviors/Visibility/Visibility.js replace with getDerivedStateFromProps(), complicated deal with this.firedCallbacks 🤔
• [x] src/modules/Transition/Transition.js replace with getDerivedStateFromProps(), complicated deal with this.nextStatus 🤔
• [x] src/modules/Transition/TransitionGroup.js replace with getDerivedStateFromProps()

Components in single move

• [ ] src/lib/AutoControlledComponent.js replace with getDerivedStateFromProps()
• [ ] src/modules/Dropdown/Dropdown.js replace with getDerivedStateFromProps()/didUpdate()
• [x] src/modules/Search/Search.js replace with getDerivedStateFromProps()

Needs work

• [x] src/modules/Sticky/Sticky.js - replace with didUpdate(), but move to <EventStack /> before

[danpantry]

Components that pass non-forwarded-ref components to <Ref /> cause <RefFindNode /> to be returned - this uses findDOMNode, which is also a violation of StrictMode.

This seems like a potentially breaking change to fix, though

[layershifter]

to be returned - this uses findDOMNode, which is also a violation of StrictMode.

@danpantry It's unfixable as there are no alternative, so we are not going to fix. Once the feature from https://github.com/reactjs/rfcs/pull/97 will be released, we will update to it.

[danpantry]

@layershifter: Hello, from the future :)

It appears that https://github.com/reactjs/rfcs/pull/97 has been closed and while there are two proposed approaches, it seems unlikely that either will land before Suspense ships.

[Celludriel]

What does this mean for now ? I'm a day into development for a small ui application at work now with semantic-ui and I am hitting this deprecation warning. If this won't be fixed I can't put code in production with warnings in the console popping up. Will it get fixed eventually ?

[danpantry]

@Celludriel The deprecation warnings are nothing to worry about unless you intend to upgrade to a version of React which removes the unsupported lifecycle methods. It doesn't appear that React will be removing them for a long time, but you should be aware that certain components that you're using within Semantic, for the time being, will not support concurrent mode and so may have subtle bugs.

If you're not using concurrent mode, it's probably not an issue.

As for whether or not it will get fixed, I imagine it will, but there problems that prevent the fix from happening right now.

[Celludriel]

@danpantry

Thanks for the information, I disabled strict mode for now so the warning doesn't pop up since our security department is pretty strict when it comes to warnings in the console. Even if it's non sensical they should worry about it.

I'll await the future if it will get fixed later on. Just glad I don't have to switch ui libraries since this one is working just fine for me now

[layershifter]

Addressing .findDOMNode() will be a complicated issue as all components should start to use forwardRef().

I am fixing UNSAFE_ warning under #3919, .findDOMNode() will be a next point.

[erasmuswill]

Hi guys. Do you need any help with this?

[brianespinosa]

I believe the rest of these deprecated methods are being resolved in #3956, after which I believe this issue can be closed.

[raymondshiner]

Any more updates on this? This feature is highly anticipated, semantic ui react in its current version throws so many warnings related to this.

[layershifter]

Progress is tracked #3919 and a lot of work was already done:

3956 is remaining to fix lifecycle warnings completely. It's quite complicated as requires tons of refactoring.

---

Until this will be funded in any way I can't allocate more time for this.

[layershifter]

Short update: #3919 was completed and all changes in master should be shipped soon.
Refs handling is a separate thing and work will be tracked under #3819.

[layershifter]

Fixes for lifecycles were released in [email protected] 🎉