davidtheclark / react-aria-modal

A fully accessible React modal built according WAI-ARIA Authoring Practices
http://davidtheclark.github.io/react-aria-modal/demo/
MIT License
1.03k stars 96 forks source link

Support for React 18 #134

Closed mdnsk closed 1 year ago

mdnsk commented 1 year ago

In this merge request I have added support for React 18 (#126).

Main changes:

mdnsk commented 1 year ago

LGTM! and looks ready to go. @mdnsk as this was initially in a draft state, let me know if this is ready.

Thank you for quick response, but there are still some problems with this MR related with refs and new React Concurrent mode. So, I need more time to explain the problem and suggest solutions.

I will mention you again, when the MR is ready.

mdnsk commented 1 year ago

This MR introduces one more breaking change affecting handling react refs in some cases.

Below is an example of component, which behavior will change after updating react-aria-modal from this MR:

class Test extends React.Component {
  constructor(props) {
    super(props);
    this.contentRef = React.createRef();
  }

  componentDidMount() {
    // with react-aria-modal@4 it will print "{ current: div }"
    // now it will print "{ current: null }"
    console.log(this.contentRef);
  }

  render() {
    return (
      <div>
        <AriaModal titleText="Test">
          <div ref={this.contentRef}>
            Modal window content...
            <br/>
            <button>ok</button>
          </div>
        </AriaModal>
      </div>
    );
  }
}

The problem can be easily fixed in the client code:

class DemoEleven extends React.Component {
  handleRef = divElement => {
    console.log(divElement);
  }

  render() {
    return (
      <div>
        <AriaModal titleText="demo eleven">
          <div ref={this.handleRef}>
            Modal window content...
            <br/>
            <button>ok</button>
          </div>
        </AriaModal>
      </div>
    );
  }
}

I found solution of similar problem in react-portal. But potentially, creating dom nodes in render function can lead to memory leak in Concurrent Mode, because calling of componentWillUnmount is not guaranteed if component wasn't mounted.

So, I suggest two variants:

@tristen, you're welcome to review the MR. Also, I haven't changed code since approval and only added this comment.