<React.StrictMode>-compliance

[erlendfh]

In reference to #1200 I've started on removing references to findDOMNode().

I've gotten ridd of all of them except one in EventContainerWrapper, which needs a larger rethink due to refs not working quite the same as findDOMNode() with components that use React.cloneElement() in render().

I've also converted the draggable context from the legacy format to using React.createContext(). A larger rewrite would probably be necessary to bring this part of the code base more in line with modern React, but this works for now.

Finally, I've replaced UNSAFE_componentWillReceiveProps() with componentDidUpdate() and getDerivedStateFromProps() equivalents.

[jquense]

thanks for working on this 👍

[jquense]

this should use static contextType where possible to avoid the extra component and instance assigning in the render fn

[jquense]

some of this should be in a getSnapshotBeforeUpdate hook, it's important that the measuring code run before render

[jquense]

see comment below

[erlendfh]

@jquense Thanks for the feedback, even learned a few new tricks in the process! Please take a look at the new commits when you have some time available. Any thoughts on the remaining findDOMNode() in EventContainerWrapper?

[jquense]

I've gotten ridd of all of them except one in EventContainerWrapper, which needs a larger rethink due to refs not working quite the same as findDOMNode() with components that use React.cloneElement() in render().

Not sure i understand specifically what you mean here. Adding a ref via cloneElement works well enough. Is the concern that it would override an existing ref, or that the ref may be passed to a function component? For the former case mergeRefs can handle it, and for the latter case, it's a fine restriction for users that their component must accept a ref and pass it to the DOM node. As a migration path, I suggest something like: https://github.com/react-bootstrap/react-overlays/blob/master/src/safeFindDOMNode.ts which will fall back to findDOMNode if needed, which allows users to migrate at their own pace

[erlendfh]

@jquense My mistake, I was of the impression that EventContainerWrapper expected an array of children, but it's a single child element, which made this a trivial fix. The examples project still has errors / warnings in the console due to an old version of Bootstrap being used, but I'm going to leave that for a separate PR as it seems at first glance to be a bit more involved than just changing the version number in package.json.

[jquense]

lets not add this to the examples

[jquense]

there are a few places where code was switched to componentDidUpdate that depended on running before render, e.g. slotMetric sets and the like.

[luanraithz]

Couldn't we throw the slotMetric stuff inside the component's state and use getDerivedStateFromProps instead of doing this manual re-initialization?

Maybe a reference

[runtothefather]

any chances to get this merged?

[ixnas]

Any updates on this? It's been a while, and it would be great to be able to use this library in StrictMode without errors.

[cutterbl]

@erlendfh Thanks for your work here. I integrated your changes with current code, taking into account @jquense feedback. The only stuff I didn't use was your DnD context stuff, for now. This is in the v1.2.0 release.

[erlendfh]

Wow, what a blast from the past! Awesome to finally get this merged though 👍🏻