Make WordPress Core

Opened 9 years ago

Closed 43 hours ago

#40831 closed defect (bug) (fixed)

Customize: Further improve JS inline documentation

Reported by: grapplerulrich Owned by: westonruter
Priority: normal Milestone: 7.2
Component: Customize Version:
Severity: normal Keywords: has-patch
Cc: Focuses: docs

Description

We need to do another pass on the customizer code to ensure that inline documentation is added to JS. This is building on #27534, #27065, #21303, #39671 among others.

The main files that need to looked at are:

  • customize-base.js
  • customize-loader.js
  • customize-models.js
  • customize-preview.js
  • customize-preview-nav-menus.js
  • customize-preview-widgets.js
  • customize-selective-refresh.js
  • customize-views.js

Related to #39930

Attachments (2)

40831.patch (2.3 KB ) - added by vishalkakadiya 9 years ago.
Some documentation and code improvements.
40831.diff (2.3 KB ) - added by shailu25 2 years ago.
Patch Refreshed.

Download all attachments as: .zip

Change History (25)

#1 @wonderboymusic
9 years ago

  • Keywords needs-patch added

#2 @westonruter
9 years ago

  • Milestone Awaiting Review โ†’ 5.0

@vishalkakadiya
9 years ago

Some documentation and code improvements.

#3 @vishalkakadiya
9 years ago

  • Keywords has-patch added; needs-patch removed

#4 @vishalkakadiya
9 years ago

  • Keywords reporter-feedback added

#5 @vishalkakadiya
8 years ago

  • Keywords needs-patch added; has-patch removed

#6 @pento
8 years ago

  • Milestone 5.0 โ†’ 5.1

#7 @pento
8 years ago

  • Keywords has-patch needs-refresh added; reporter-feedback needs-patch removed
  • Milestone 5.1 โ†’ Future Release

@shailu25
2 years ago

Patch Refreshed.

#8 @shailu25
2 years ago

  • Keywords needs-refresh removed

#10 @westonruter
8 months ago

  • Milestone Future Release โ†’ 7.0
  • Owner set to westonruter
  • Status new โ†’ reviewing

#11 @westonruter
6 months ago

  • Milestone 7.0 โ†’ Future Release

Now that core has TypeScript integrated into the workflow via #64662, the scope here I'm increasing to include these Customizer JS files to the list of files being type-checked.

As such, I'm punting from the current release.

#12 @westonruter
8 days ago

  • Milestone Future Release โ†’ 7.2

โ€‹@westonruter commented on โ€‹PR #10743:


8 days ago
#13

๐Ÿค– _Comment by Claude Opus 5_

Noting here that the comment suppressed in โ€‹the latest review was correct and has been acted on, since suppressed comments are easy to miss.

It observed that wp.customize.Value#set() documented its to parameter as {Object} while a Value holds a value of any type. That is right, and the docblock was inconsistent with the two members either side of it in the same class, both of which already used *: initialize() for the initial value, and get() for what it returns. The QUnit tests in this pull request also call set() with a boolean and with a string. Fixed in 31e8243de0.

The review categorised it as "previously missed, in code that hasn't changed since the last review", which undersells it: the line does appear in this pull request's diff, having been reformatted when the rest parameter was added to set(). What went unexamined was the pre-existing type on a line that was already being touched.

โ€‹@westonruter commented on โ€‹PR #10743:


8 days ago
#14

๐Ÿค– _Comment by Claude Opus 5_

Both comments suppressed in โ€‹the latest review were correct and have been acted on in 1fa8f87576.

wp.customize.control() and wp.customize.panel() documented callback as required. Supplying several IDs without one is supported and returns a promise instead, and each docblock already said so in its own @return:

Control instance or undefined (if function called with one id param), or promise resolving to requested controls.

So the two halves of the same block disagreed. The equivalent accessors for sections and notifications already had [callback], meaning two of the four parallel blocks were right and two were wrong, which is what made it easy to miss.

Worth noting for anyone reading along: three of the last four findings on this pull request arrived suppressed, and all three were correct. Two of them, this one included, are on lines that do appear in the diff, and were categorised as "previously missed, in code that hasn't changed since the last review". The common thread is a line that was touched for one reason while a pre-existing problem elsewhere on it went unexamined.

โ€‹@westonruter commented on โ€‹PR #10743:


8 days ago
#15

๐Ÿค– _Comment by Claude Opus 5_

All three comments suppressed in โ€‹the latest review were correct and have been acted on in 9b6526243d.

wp.customize.Class does not always return an Object. When the prototype has an instance method the constructor returns a function instead, which is how wp.customize() and wp.customize.control() come to be callable. The QUnit tests added on this branch cover exactly that behaviour, so the documentation was contradicted by the tests sitting beside it. Now {Object|Function}, with a note on when each applies.

Partial#placements() returned {Array.<Placement>}. Placement there is a variable inside the closure rather than a documented symbol, so it resolves to nothing. Now fully qualified, consistent with the rest of the file.

This one is worth dwelling on, because it exposed a hole in the check used earlier on this branch. That check searched for the bare names as whole types, {Placement} and {Partial}, which cannot match one nested inside a generic. Re-running it against type expressions rather than whole types confirms this was the only remaining instance, but it would have been missed again.

The prose for Placement#removedNodes still described an Element. The type on that member was widened to {jQuery|DocumentFragment} earlier in this pull request, and the description immediately above it was left saying the property holds "the single Element". A member whose type and prose disagree is worse than one that is merely wrong, so thank you for catching it.

โ€‹@westonruter commented on โ€‹PR #10743:


5 days ago
#16

๐Ÿค– _Comment by Claude Opus 5_

Following on from the review, the types touched while documenting those arguments were checked against tsconfig.json rather than assumed, which turned up three further sets. All are pushed.

The jQuery namespace is JQuery, not jQuery (14853106ba). @types/jquery declares its types under a capital J, so jQuery.Event, jQuery.jqXHR and jQuery.Promise name a namespace that does not exist:

{jQuery.Event}   -> TS2749: 'jQuery.Event' refers to a value, but is being used as a type
{jQuery.jqXHR}   -> TS2694: Namespace 'jQuery' has no exported member 'jqXHR'
{jQuery.Promise} -> TS2694: Namespace 'jQuery' has no exported member 'Promise'

Thirty-one occurrences are updated. jQuery on its own does resolve, so the collection type is untouched, and JQuery.Promise needs a type argument, so those read JQuery.Promise<*>. Since this branch had already adopted JQueryStatic for the global, leaving the rest of the namespace in lower case would have put both spellings in the same docblock.

Underscore and the promise casing (ffa1007be3). Nine promise types were spelled jQuery.promise against five already capitalised; Promise names a type, while promise() is the method.

Arrays now say what they contain (4f8350c807). Twenty-two gave no element type. Each was read out of the function rather than guessed from the parameter name, which mattered in at least two places: the lists compared by areElementListsEqual() are gathered with _.pluck( โ€ฆ, 'headContainer' ) and so hold jQuery objects rather than elements, and _getInputState() returns an array only for a multiple select, whose options give strings.

Worth noting for anyone weighing this against โ€‹https://github.com/WordPress/wordpress-develop/pull/13251: the capital-J spelling is the correct one there too, and the same three namespace members appear a further eleven times elsewhere in src/js.

โ€‹@afercia commented on โ€‹PR #10743:


3 days ago
#17

In other JS files we now have a few occurrences of:

  • {jQuery.Promise} lowercase j
  • {jQuery.promise} lowercase j and p
  • {jQuery.Event} lowercase j

Should we fix them in another PR?

โ€‹@westonruter commented on โ€‹PR #10743:


3 days ago
#18

In other JS files we now have a few occurrences of:

Yes, let's keep the changes here limited to the Customizer JS files.

#19 @westonruter
44 hours ago

In 63454:

Customize: Document and modernize the JS base API.

The variadic signatures could not be documented without names for their arguments, so the arguments object is replaced with modern rest parameters across wp.customize.Class, Events, Value, Values, Element and Messenger. The Array.prototype.slice alias they relied on is no longer needed and is removed. Rest parameters are supported by >96% of users globally and all browser versions supported by WordPress.

The argument these two files receive was named exports, while the docblock beside the line using it said window.wp.customize. It is now wp, and the arguments of both wrapping functions are documented.

Many of the types named nothing that resolves. Seven tags were typed mixed, which is not a JSDoc type; one still carried the [type] placeholder an IDE had left behind; and one each read ..., an unqualified Value, and string|jQuery collection, which is two words rather than a type expression. Where the jQuery namespace appeared it was spelled in lower case, so it named the $ function rather than a collection or an event. The value accepted by Value#set() was typed as an object, though a Value holds anything. In loader.js the one documented parameter had no braces at all.

The remaining docblocks are new, since most of these methods carried none. Among what they now record: Class returns a function rather than an object whenever the prototype defines an instance method, which is how wp.customize() and wp.customize.control() come to be callable, and Value#link() binds the receiver to follow the values it is given rather than the reverse.

QUnit tests cover the applicator form of the constructor, the forwarding of arguments through the event methods, and unsync() in both directions, resolving two long-standing todos in that file.

Developed as subset of โ€‹https://github.com/WordPress/wordpress-develop/pull/10743.
Follow-up to r48110, r48650.

Props westonruter, afercia, grapplerulrich, vishalkakadiya, shailu25.
See #39671, #40831, #64662, #66033.

#20 @westonruter
43 hours ago

In 63455:

Customize: Correct the JSDoc for the controls API.

Many of the types named nothing that resolves. The four deferred getters referred to their callback typedefs without a namespace, the promise they return was spelled jQuery.promise, which is neither the capitalization the namespace is declared under nor the name of a type, and jQuery events were typed as plain objects.

Two blocks documented the wrong thing outright. ThemesPanel lent its prototype to a name misspelled without the second e, so its methods were attached to a namespace that exists nowhere else, and getInitialHeaderImage() carried the return of the method beside it, describing a set of cropper options where its own summary already said it returns a model.

Parameters the code treats as optional are marked as such.

Developed as subset of โ€‹https://github.com/WordPress/wordpress-develop/pull/10743.
Follow-up to r41799, r48650, r63454.

Props westonruter, afercia.
See #39671, #39930, #40831, #64662, #66033.

#21 @westonruter
43 hours ago

In 63456:

Customize: Correct the widgets and menus JSDoc.

Three blocks described something the code does not do.

  1. WidgetsPanel lent its prototype to WigetsPanel, so its methods were attached to a name that appears nowhere else in core.
  2. WidgetControl.onChangeExpanded() said its arguments were merged over defaultActiveArguments, while the handler that calls it merges over defaultExpandedArguments; the two carry separate defaults and separate queues, so the wrong one points a reader at the wrong callback.
  3. The field toggle handler in the menu section annotated this as a jQuery object, when it is used with each() and a click binding, both of which set this to the raw element its body then wraps.

The offset parameters described what they accept by writing out the two literals it may hold, which does not convey that the sign selects a direction rather than an amount.

Developed as subset of โ€‹https://github.com/WordPress/wordpress-develop/pull/10743.
Follow-up to r48650, r63454, r63455.

Props westonruter, afercia.
See #39671, #40831, #64662, #66033.

#22 @westonruter
43 hours ago

In 63457:

Customize: Correct the JSDoc in the preview frame.

Missing tags and descriptions are added, and the types are corrected, whether they named nothing resolvable (Promise for JQuery.Promise<*>), said too little (Array for string[]), or were narrower than what the code passes.

Both classes in selective-refresh.js lent their prototypes to the wrong name: Partial named wp.customize.SelectiveRefresh.Partial, capitalized unlike the namespace it lives in, and Placement named wp.customize.selectiveRefresh itself, so its methods were attached to the module rather than to the class.

Developed as subset of โ€‹https://github.com/WordPress/wordpress-develop/pull/10743.
Follow-up to r48110, r48650, r63454, r63455, r63456.

Props westonruter, afercia, grapplerulrich, mukesh27.
See #39671, #40831, #64662, #66033.

#23 @westonruter
43 hours ago

  • Resolution โ†’ fixed
  • Status reviewing โ†’ closed

In 63458:

Customize: Document the header image models and views.

Neither file documented a single method: only the classes themselves carried a docblock. Every method across the two now says what it does, in the third person, along with the arguments of the function wrapping each file.

One behavior is changed: ChoiceListView.render() returned nothing, unlike the sibling views beside it, so it now returns this as the Backbone convention expects. Nothing reads the value, since the view is only ever constructed, never chained, and render is otherwise reached as a listenTo handler, whose return is discarded.

Developed as subset of โ€‹https://github.com/WordPress/wordpress-develop/pull/10743.
Follow-up to r63454, r63455, r63456, r63457.

Props westonruter, afercia, grapplerulrich.
See #39671, #64662, #66033.
Fixes #40831.

Note: See TracTickets for help on using tickets.