Removed usages of valueLink from client/me/account

[jsnajdr]

Removes usage of valueLink from client/me/account/main.jsx.

Similar work has already been done when Reduxifying client/my-sites/site-settings and even after this PR, remains to be done in the rest of the client/me directory.

Will be useful for PR #12544 - new LanguageChooser component that no longer needs to support dual API now.

The commit messages contain additional details.

Testing instructions:
Test if all the form elements in client/me/account still work properly, especially:

• changing the language and enabling/disabling the community translator
• changing the username, with its "confirm username" workflow
• changing the email address, with its confirmation workflow
• enabling/disabling the "holiday snow" feature :)

[enejb]

I tested this and when I load http://calypso.localhost:3000/me/account locally

I am seeing the following js errors.

I don't see them when I am visit the page on production.

[spen]

I'm seeing errors similar to @enejb's but everything seems to be working.
I've left a few comments though as there are some things that I'm not sure about - they're not blockers though - interested in hearing your thoughts on them :)

[spen]

I'm wondering if it makes sense to add these methods to the component itself.
An alternative approach that would avoid repeating this.props.userSettings.getSetting without adding methods to the component would be something like this:

render() {
  const { userSettings, ... } = this.props;
  const { getSetting } = userSettings;
  ...
  value={ getSetting( 'user_login' ) }
  ...
}

Actually, the current usage in render seems a nice pattern. As it's clearer with userSettings.getSetting which settings we're referencing.

[spen]

If we do decide to keep getUserSetting & updateUserSetting, could we use this.updateUserSetting() here?
Also should we use those methods throughout the rest of the component?

[spen]

To me, a method named updateUserSetting would take two args, one being the target and one being the value, especially when considering getUserSetting's relationship to userSettings.
I wouldn't intuitively guess that it returns another function either.

I'd rather see updateUserSetting be more general rather than assuming that it will always be used in the context of an event callback.
A suggestion of a more general implementation might be something like this:

updateUserSetting( name, value ) {
  return this.props.userSettings.updateSetting( name, value )
},
...
someOtherMethod() {
  ...
  onChange={ event => this.updateUserSetting( 'setting', event.target.value ) }
  ...
}

Of course, that means there's more repetition ( of the event => this.x( ... ) pattern) but I think this is a fair trade-off for extra clarity and flexibility.

Perhaps renaming the method to suggest that it returns another function (that is essentially updateUserSetting) might be an idea - though I'm not sure what this would look like :/

But, to be clear, I'm still not sure I agree that they should be added to the component...
I'm interested to hear some other peoples thoughts on this though.

[spen]

lodash.isEmpty Could be a good fit for checking the value here.

[jsnajdr]

In this case, the event.target is a HTMLInputElement and its value property is guaranteed to be a DOMString - it's never null or anything else.

[spen]

This pattern further suggests to me that this generalisation shouldn't really be made.
updateUserSetting can't be used in a general way so we're having to add updateUserSettingCheckbox to compensate for checkbox's nuance.

[jsnajdr]

Hi @spen! Thank you for looking at the patch so thoroughly.

In addition to removing valueLink usages, this PR is a first step in a larger refactoring plan whose goal is to remove the userSettings module and replace it with Redux state.

For this reason, I don't want to use this.userSettings directly, but rather abstract it away to methods like getUserSetting or updateUserSetting. In one of the follow-up PRs, their implementations will change to use Redux selectors and action creators instead.

When passing an onChange prop to a React component, Calypso's eslint rules prohibit using arrow functions or bind in the prop value:

  onChange={ event => this.updateSetting( 'x', event.target.value ) } // error
  onChange={ this.updateSetting.bind( this, 'x' ) } // error
  onChange={ this.updateSetting } // OK
  onChange={ this.updateSettingCurried( 'x' ) } // OK

This has good reasons - it forces us to move complexity out of the JSX markup and also often improves performance - arrow functions and bound functions would get re-created on every render call.

It's also the reason why the updateUserSetting method needs to be a curried function - instead of two parameters, it an one-parameter function that returns another function that takes the second param.

I agree that putting the getUserSetting and updateUserSetting methods into the Accounts class is less than ideal. Ultimately, they should move to a new client/me/wrap-settings-form.jsx component that would be reused by other components in the client/me directory. The client/my-sites and client/extensions/wp-super-cache directories already use this HOC approach. That's also the place where I learned about the curried event handler technique :)

I'm pushing a new version of the PR with the following changes:

• I fixed the React warnings mentioned by @enejb and you by putting || '' at the right places.
• I created a method updateUserSetting(settingName, value) (a simple, no-surprise one) and two other curried ones that are optimized to be a good onChange handlers: updateUserSettingInput and updateUserSettingCheckbox. I hope this change removes some of the confusion.
• I use getUserSetting and updateUserSetting consistently across the whole Account component, as you suggested in one of your comments. There are still some remaining direct accesses to other userSettings methods, but they will eventually go away, too.

Thanks again for the review and let me know what you think about the new version!

[spen]

Thanks for the detailed response and for the updates @jsnajdr!

Knowing the motivations makes the abstractions make more sense :)

My concern with getting linting to pass with a curried method is that this feels more like a bypass than a solution.
This is just a general point - not saying it's wrong to do this as I see we do it across the project.

// As you mention, this wouldn't pass lint, 
// because we want to avoid binding a new function on each render
onChange={ event => this.updateSetting( 'x', event.target.value ) }

// But currying like so is essentially doing the same thing
// in terms of creating & binding a new function - only the linter doesn't see that.
onChange={ this.curryUpdateSetting( 'x' ) }

When this is eventually reduxified, would we connect with the methods pre-curried or curry them on the fly?

An example of what I mean:

// very much pseudo code
connect( 
  mapState...,
  dispatch => ( {
    updateUserURL: updateSetting( 'user_URL' ),
    updateUserEmail: updateSetting( 'email' ),
    ...
  } )
} );

If so we could implement connect or a similar HoC early and move the abstractions there.

By the way, I should be clear that I'm just thinking out loud - I'm not saying any of these points are blockers/rejections :)

[jsnajdr]

@dmsnell Would you mind having a quick look at this PR and giving me a formal r+ from a staff A11n? It's part of the user setting Reduxization project, like the #12819 PR you also reviewed for me. Thanks!

[dmsnell]

just a note on this here related to the previous discussion in #12729 (comment)

moving the closure generator into a separate function doesn't actually do anything different than inlining the arrow expression inside of the JSX. so while we have made the linter happy by extracting it we haven't actually addressed the reason that linting is there in the first place

when we need to respond to events and pass data into that handler then the ideal situation is simply to leave the information in the JSX and stop creating a new handler on every call to render()

updateUserSetting = event => {
	const value = event.target.value;
	const setting = event.target.dataset.settingName

	update( setting, value );
}

<FormTextInput
	…
	data-setting-name="user_URL"
	onChange={ this.updateUserSetting }
/>

note the use of the custom data attributes to hold the additional properties

[jsnajdr]

I think it's also very useful if the linter doesn't let you put too much complex code into a JSX attribute, and forces you to extract the logic to a separate method. Even if there is no performance benefit from that.

But anyway, putting the settingName to the JSX markup is a great idea! We don't even need additional data attributes, as the name is already there in the id and name properties.

I updated the id, name and htmlFor attribute values to be consistent with the userSettings property names, and rewrote the updateUserSetting* methods to get the setting name from event.target.name.

[dmsnell]

I'm not sure the commit with these changes came through…

[dmsnell]

I think it's also very useful if the linter doesn't let you put too much complex code into a JSX attribute, and forces you to extract the logic to a separate method. Even if there is no performance benefit from that.

this just isn't the point of the linting rule though. it's specifically there to catch a performance and behavioral issue that's easy to overlook and hard to debug. creating new closures in the props of a React component can do weird things and that's true whether by using the arrow functions inline or by calling a function which returns a new function.

we need to remember that there is no sense of a value comparison of functions. every time render() gets called here we create a new function closure and even if all other things are identical that won't shallow-compare to equality with the previous run. this is both a performance issue and can trigger re-renders and it's also a bug vector because any state created in the function gets lost as we abandon the old function invocation.

[jsnajdr]

I'm not sure the commit with these changes came through…

I squashed these changes into the "Removed usage of valueLink from client/me/account" and did push -f. The Github UI is showing me the right version now.

[jsnajdr]

we need to remember that there is no sense of a value comparison of functions. every time render() gets called here we create a new function closure and even if all other things are identical that won't shallow-compare to equality with the previous run

Thanks for explaining this! The old-style non-Redux Calypso code still contains a lot of performance gotchas like this. For example, the Account component will still use eventRecorder mixin at places like:

onFocus={ this.recordFocusEvent( 'Email Address Field' ) }

And all these subtle performance problems will kind of magically disappear during the Reduxification!

[dmsnell]

this looks very old and I don't usually see new with debug

you might just consider matching a common pattern from Calypso (but this isn't necessary)

import debugFactory from 'debug';

const debug = debugFactory( 'calypso:me:account' );

[jsnajdr]

Good catch! Updated this in a new micro-commit in this PR.

[dmsnell]

note that we have introduced an opening for TypeErrors when userSettings isn't an object with a getSetting value

as a function in the module scope we could generalize this in a safe way…

const getSetting = ( props, name ) => invoke( props, 'userSettings.getSetting', name );

…

getSetting( this.props, 'user_login' );

[jsnajdr]

Hmm, is there any rule-of-thumb to determine when the code should be defensive like this, and when to just trust the inputs?

After all, in a dynamic language like JS, one can never be sure about the type and shape of anything.

There is a lot of method calls on the this.props.userSettings and this.props.username in this module, so it would make sense to wrap them all in invoke.

The only place where this component is instantiated is the controller.js in the same directory. These two components are tightly-coupled "friends", so I think that the client/me/account/main.jsx component shouldn't need to worry if its props have the right type. What do you think?

[dmsnell]

is there any rule-of-thumb to determine when the code should be defensive like this, and when to just trust the inputs?

yes: don't introduce the opportunity for TypeErrors of this kind 😉

[dmsnell]

There is a lot of method calls on the this.props.userSettings and this.props.username in this module, so it would make sense to wrap them all in invoke.

The only place where this component is instantiated is the controller.js in the same directory. > These two components are tightly-coupled "friends", so I think that the client/me/account/main.jsx component shouldn't need to worry if its props have the right type. > What do you think?

we can be easier with props, but we should also be declaring propTypes and listing which ones are required. this.props is safe and so is this.props.someValue because even if someValue doesn't exist we are fine. what isn't okay is this.props.someValue.someProperty because if someValue isn't an object now we have an actual browser error

[jsnajdr]

Just pushed a commit where the Account component has propTypes declared and where all userSettings and username method calls are wrapped in invoke.

[dmsnell]

see note below, but we should probably nix this method. it appears to only exist as a workaround to avoid linter messages

[dmsnell]

see above and below comments on working around the linter

[dmsnell]

is this even necessary? I couldn't find any use of this.state.redirect

[jsnajdr]

Yes, it is used by the formBase mixin: https://github.com/Automattic/wp-calypso/blob/master/client/me/form-base/index.js#L81-L86

I'd like to get rid of the redirect eventually and apply the new language without reload, but that's another story (and PR).

[dmsnell]

in this case we could simplify this branching a bit

const redirect = ( value !== originalLanguage ) && '/me/account'; // false if languages match
this.setState( { redirect } );

[dmsnell]

this conditional structure is getting a little big. I wonder if we could make it less noisy…

const emailValidationError = (
	( '' === value && 'empty' ) ||
	( ! emailValidator.validate( value ) && 'invalid' ) ||
	false
);

this.setState( { emailValidationError } );

[jsnajdr]

I'm not sure if this is really an improvement. The if/else chain might be verbose, but it's instantly clear what it's doing.

Maybe it would help if the branches were just setting a variable and there was one call to this.setState at the end?

Of course, this would be a great use case for a pattern-matching switch statement (like match in Rust) 😄

[dmsnell]

@jsnajdr we might have other options. I was just trying to point out that the conditional structure's noise obscures the meaning at least to me. a separate function could help. additionally, we just prefer to keep else statements out as much as possible

const toError = value => {
	if ( '' === value ) {
		return 'empty';
	}

	if ( ! emailValidator.validate( value ) ) {
		return 'invalid';
	}

	return false;
}
const emailValidationError = toError( value );

we could say in general that it's an odd construct returning a mixture of data types when comparing one value against a computed value

[jsnajdr]

My first impression wasn't great, but now I'm starting to like the compact code with the && and ||. Pushed a commit where the redirect and emailValidationError state updates are rewritten in this style.

[dmsnell]

the false in my original concoction wasn't even necessary because if the first two conditions fail the result will be false but I left it in there to try and indicate more explicitly what would happen. that's really quite a difficult thing the way it's checking

validationError = isEmpty `mplus` isInvalid
	where
		isEmpty = if "" == value then Just "empty" else Nothing
		isInvalid = if (not . validate) value then Just "invalid" else Nothing

[jsnajdr]

The explicit false makes the code more readable, as it makes the structure of the expression more similar to a switch statement. I'd leave it there.

[dmsnell]

@jsnajdr I'm going to back up a little bit here as I think I may have miscommunicated a bit how strongly I suggested replacing invocations. just a few more thoughts: there are some in there that I think are probably appropriate, others that I think might be needless to replace.

definitely if we come to a spot where a previous call would have failed we can't assume that ignoring the error means we're safe. we might as well make further calls direct, better still to validate that the thing exists before the sequence of method invocations.

maybe if I can think more clearly I can write it out better

[jsnajdr]

@dmsnell I guess I don't fully understand what problem we are trying to solve with these invoke wrappers. If the component is passed wrong props, should the component silently recover from that and avoid throwing a TypeError at all costs? Isn't it better to expose the potential bug and simply solve it by passing the right props?

I feel that we're guarding against an error that has almost no chance to really happen. And when we migrate the component to Redux, the code that works with the userSettings and username will change anyway - there will be selectors and action creators passed down by connect.

It would help if you could provide some context. Did a similar bug happen in the past? I see a handful of invoke wrappers in the codebase, but it's certainly not a regular pattern.

[dmsnell]

@jsnajdr looking at the bug report in Slack I see what amounts to over 20,000 known errors raised in customers' browsers yesterday due to attempted property access of undefined or null values.

we should never ignore errors, but also shouldn't flood our customers with those errors.

my best recommendation here is to simply spend some time to think about the consequences of each property access and the following sequence if one fails. bugs pop in when we're sure something couldn't fail, because if we thought they would happen we would prevent them.

the specific problem we are trying to solve is throwing errors in our customers' browsers and further killing the currently-executing JavaScript thread as a result, thus preventing other intended actions from occurring.

[jsnajdr]

@dmsnell I gave up on wrapping the userSettings and username property accesses with invoke. It doesn't really help to prevent a TypeError when I pass a null prop accidentally. There are still TypeErrors coming up when lib/mixins/data-observe is attaching event handlers etc.

Instead, I just added PropTypes definitions that declare userSettings and username as required props of type object.

[dmsnell]

for funnies we could also rewrite this…

getEmailAddress() {
	return this.getUserSetting(
		this.hasPendingEmailChange()
			? 'new_user_email'
			: 'user_email'
	);
}

// or, maybe better

getEmailAddress() {
	const setting = this.hasPendingEmailChange()
		? 'new_user_email'
		: 'user_email';

	return this.getUserSetting( setting );
}

[jsnajdr]

I'll implement this suggestion in a later PR that will Reduxify the form. This code will change to calling selectors anyway.

[spen]

Something like @dmsnell's second would get my vote - but ¯\_(ツ)_/¯ since changes are planned in the near future!

[dmsnell]

any particular reason for changing the names of this and other fields? maybe something better for a different PR?

[jsnajdr]

This makes the name field of the <input> identical to the setting name. The updateUserSettingInput can then pick up both the name and the value from the event.target object. We discussed this in one of the earlier comments on this PR.

[dmsnell]

the code looks fine to me. I'll back out of the review process and let @spen or others give the final look-over 👍

[spen]

Something like @dmsnell's second would get my vote - but ¯\_(ツ)_/¯ since changes are planned in the near future!

[spen]

Would there be any value to type-casting this with !!?
I would imagine not but worth consideration.

[jsnajdr]

I think it's not needed - the checked setter on the DOM element accepts any value and converts it to boolean. And the holidaysnow setting is pretty much guaranteed to be a boolean already.

[spen]

A tiny nit here:

// Might this be a little clearer?
const isSubmitButtonDisabled = (
  ! userSettings.hasUnsavedSettings() || 
  this.getDisabledState() || 
  this.hasEmailValidationError()
);

(Not a blocker)

[spen]

Another little nit here, but if we bump this.state.submittingForm to be checked first we can avoid calling the functions in the other two checks.

(Not a blocker since it's so, so minor)