Toggling any module ON on Jetpack settings results in React warning

[manzoorwanijk]

Impacted plugin

Jetpack

Quick summary

When a Jetpack module is OFF and you try to turn it ON on Jetpack settings page, it results in the warning:

A component is changing an uncontrolled input to be controlled.

Steps to reproduce

• Goto Jetpack settings, e.g. /wp-admin/admin.php?page=jetpack#/discussion
• Enable a module, e.g. Comments
• Notice the warning in the browser console.

A clear and concise description of what you expected to happen.

There should be no warning.

What actually happened

A warning is shown in browser console

Impact

All

Available workarounds?

No and the platform is unusable

If the above answer is "Yes...", outline the workaround.

No response

Platform (Simple and/or Atomic)

Self-hosted, Atomic

Logs or notes

The bug seems to have been there for six years and the source of that bug is the OR (||) operator here. It should instead be a null coalescing operator.

jetpack/projects/plugins/jetpack/_inc/client/components/module-toggle/index.jsx

Line 99 in 5e41cdf

checked={ this.props.activated || this.props.isModuleActivated }

- checked={ this.props.activated || this.props.isModuleActivated }
+ checked={ this.props.activated ?? this.props.isModuleActivated }

[anomiex]

I note that ?? will give different results for two cases. One is what you intend, but the other probably isn't. And one that probably should be changed isn't being changed either, although due to

jetpack/projects/plugins/jetpack/_inc/client/components/module-toggle/index.jsx

Line 24 in 5e41cdf

activated: false,

that case may not be able to happen.

this.props.activated	this.props.isModuleActivated	||	??
undefined	undefined	undefined	undefined
undefined	true	true	true
undefined	false	false	false
true	undefined	true	true
true	true	true	true
true	false	true	true
false	undefined	undefined	false
false	true	true	false
false	false	false	false

In other words, if this.props.activated is false while this.props.isModuleActivated is true, changing to ?? will change it from checked to unchecked.

If we don't want that change, using this.props.activated || this.props.isModuleActivated || false, or ensuring that isModuleActivated is not undefined, or if activated already can't be undefined swapping it to this.props.isModuleActivated || this.props.activated would work better.

Or another possibility is that isModuleActivated never seems to be used in the first place, and I wonder if #8934 had intended for it to be used as something like this.props.isModuleActivated( this.props.slug ). We could just drop it entirely.

[manzoorwanijk]

Yeah, I'm not sure about the logic here. ?? was just a thought.
Yeah, it will be good to get rid of that second condition.