New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React: Remove the usage of the componentWillMount lifecycle #7282

Merged
merged 3 commits into from Jun 19, 2018

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Jun 12, 2018

Together with #7280 #7206 This PR drops the componentWillMount lifecycle method from all our components.

Testing instructions

  • check that popovers return focus properly to the called (keyboard navigation)
  • check that fills show up properly (block toolbars for instance)

@youknowriad youknowriad requested review from gziolo and aduth Jun 12, 2018

@youknowriad youknowriad self-assigned this Jun 12, 2018

@gziolo

gziolo approved these changes Jun 12, 2018

LGTM, it would be nice to wait for @aduth to confirm the part where serializer changes.

@@ -441,10 +441,6 @@ export function renderNativeComponent( type, props, context = {} ) {
export function renderComponent( Component, props, context = {} ) {
const instance = new Component( props, context );
if ( typeof instance.componentWillMount === 'function' ) {
instance.componentWillMount();

This comment has been minimized.

@aduth

aduth Jun 12, 2018

Member

Is it in our interest to drop this? Seems like it'll be supported by React for at least the near future, and could otherwise introduce some unintended regressions to not have respected by the serializer.

This comment has been minimized.

@youknowriad

youknowriad Jun 12, 2018

Contributor

I thought we should discourage it even if React still supports it, I don't expect using a Component class in save functions at all Actually. but I can bring it back if needed.

This comment has been minimized.

@aduth

aduth Jun 12, 2018

Member

Maybe for now introduce it as deprecated, since there is a chance (albeit small) that a developer is already relying on it in their own blocks.

This comment has been minimized.

@gziolo

gziolo Jun 12, 2018

Member

They might see doubled warnings, one from us and one from React, but it still makes sense

This comment has been minimized.

@aduth

aduth Jun 12, 2018

Member

I might think React's will not warn since it is never mounted / initialized in the serialization step?

class Example extends Component {
constructor() {
super( ...arguments );
this.constructed = 'constructed';
}
componentWillMount() {

This comment has been minimized.

@gziolo

gziolo Jun 19, 2018

Member

@youknowriad, let's keep this test unchanged and remove it together with deprecation as part of 3.3 release.

I think you can merge it after this change is reverted. I guess you will have to update the test to catch the warning.

@youknowriad youknowriad merged commit f0b0aac into master Jun 19, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.04% but relative coverage increased by +53.31% compared to 14531c4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/drop-component-will-mount branch Jun 19, 2018

@youknowriad youknowriad added this to the 3.1 milestone Jun 19, 2018

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