Skip to content

Conversation

@kylehalleman
Copy link

With React 16.3 componentWillMount was deprecated and a StrictMode component was introduced to help identify potential issues for the future async rendering in React.

To make this project compatible with StrictMode, I moved the code in componentWillMount to the constructor. Looking at the code in componentWillMount it was safe to move it to constructor since it is merely setting an instance variable and not worrying about props changes.

If this PR is approved and merged, I plan to make a similar PR to react-aria-modal to update it to support StrictMode.

@toolness
Copy link

This is awesome @kylehalleman, thanks for contributing it!

My only concern is with server-side rendering. While I know that this component probably isn't made for SSR, it'd be nice if it could at least render without throwing an exception. During SSR, I think that lifecycle methods like componentWillMount won't be called, but by moving all its logic to the constructor, it will be called on the server--and because it assumes document is available, it will throw an exception on the server.

Do you think there's any way to make this not throw an exception on the server-side?

@kylehalleman
Copy link
Author

Two approaches I could see addressing this. A simple one would be to check for document in the constructor and not run if it's on the server and then run again on componentDidMount. Another approach might be moving this logic to componentDidMount and then handling for cases in render where this.container is not yet set.

Thoughts?

@toolness
Copy link

Good ideas!

Because componentDidMount() only ever runs after the component has been added to the DOM, I often use it as a place to put any initialization logic that requires the DOM (or any other APIs I know won't be available on the server), so it's what makes the most sense to me. However, I'm not sure if that's what the React community at large thinks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants