Review Customizer Nav Menus JS

[wonderboymusic]

A few notes:

• The $.Deferred is unnecessary - when resolved, it just calls init, which adds another callback to the stack, which then gets called next. All of these can be combined into one.
• self is a bad local variable name. Sometimes self is a substitute for this when scope is lost in a maze of callbacks, but most of the time, it just makes this more ambiguous than it already is.
• Almost all of these functions are static helper methods, do any of them need to exposed publicly? We have gotten into a bad habit of globalizing all of our JS, which has made BC a living hell when we want to use things like browserify. I would expose as little as possible everywhere.
• We should distinguish between settings and class/instance properties.

The patch makes this JS look more like the other Customizer JS. I am not going to drop it in right now. I may have to wait until all of the Customizer turbulence is over and then do one cleanup.

[westonruter]

• Almost all of these functions are static helper methods, do any of them need to exposed publicly? We have gotten into a bad habit of globalizing all of our JS, which has made BC a living hell when we want to use things like browserify. I would expose as little as possible everywhere.

By exposing these publicly, we can unit test them (which still needs to be done, per #32688). So that's an advantage.

• self is a bad local variable name. Sometimes self is a substitute for this when scope is lost in a maze of callbacks, but most of the time, it just makes this more ambiguous than it already is.

Agreed. We should use setting, control, or some other name that is more semantic.

[westonruter]

• The $.Deferred is unnecessary - when resolved, it just calls init, which adds another callback to the stack, which then gets called next. All of these can be combined into one.

I've eliminated this in the patch for #32894.

[westonruter]

In 33134:

Customizer: Defer listening to nav menu setting changes until active.

Defer the preview starting to listen to setting changes until after the Customizer has synced settings into the preview. This ensures that any differences between the JS and PHP representations of the settings won't cause an infinite refresh.

See #32911.
Fixes #32894.

[obenland]

Is there anything left to do here @wonderboymusic, or can it be closed?

[wonderboymusic]

In 33345:

Customizer: Nav Menus JS cleanup, first round

• In init, settings should be fresh and set to default values before extending
• When looping via _.each, pass this as 3rd arg to bind context
• Settings should be encapsulated in the module, instead of being exposed as static class props
• currentRefreshMenuInstanceDebouncedCalls should be encapsulated and renamed, instead of being the longest symbol in the entire codebase

See #32911.

[wonderboymusic]

In 33347:

Customizer: Nav Menus JS cleanup, second round

• Follow the same pattern of namespace instantiation that WidgetCustomizerPreview uses
• Remove use of self for global delegation
• Use api for wp.customize and import only top-level globals
• Bind this where appropriate and disambiguate scope

See #32911.