Configure Babel to remove non-public comments

[colinrotherham]

This PR takes a look at component KB file size without comments

It configures Babel to remove all non-public comments unless tagged with @preserve or containing JSDoc tags

Before

v5.0.0-preview

File sizes from the main branch

Component module	File size
all.mjs	114.7 KB
accordion.mjs	40.31 KB
character-count.mjs	39.06 KB

v4.7.0

File sizes from the review app on the v4.7.0 tag

Component module	File size
all.mjs	161.40 KB
accordion.mjs	82.67 KB
character-count.mjs	76.34 KB

After

Only non-public comments removed

File sizes from rollup-babel-comments branch with all non-public comments removed versus v5.0.0-preview

Component module	File size	Change
all.mjs	66.35 KB	-42.15%
accordion.mjs	20.77 KB	-48.47%
character-count.mjs	20.34 KB	-47.93%

Only private and internal comments removed

File sizes from a previous spike with private and internal comments removed versus v5.0.0-preview

Component module	File size	Change
all.mjs	94.19 KB	-17.88%
accordion.mjs	36 KB	-10.69%
character-count.mjs	34.13 KB	-12.62%

All comments removed

File sizes from a previous spike with ALL comments removed versus v5.0.0-preview

Component module	File size	Change
all.mjs	55.57 KB	-51.55%
accordion.mjs	19.98 KB	-50.43%
character-count.mjs	15.24 KB	-60.98%

[romaricpascal]

Really like the ideas of only keeping the comments that we need. A couple of questions as I looked into it:

• why do we need to preserve the comments for I18n? And more generally, what would be the rule of thumbs for using @preserve?
• do you think it'd be worth/possible to automate the addition of @preserve? I was thinking of something like adding it to any JSDoc block that doesn't have an @private using a Babel transform, but that likely depends on the rules for what needs that @preserve tag.

[colinrotherham]

Thanks @romaricpascal

• why do we need to preserve the comments for I18n? And more generally, what would be the rule of thumbs for using @preserve?

There are a few things flagged @private that can't really be 😣

For example, the JSDoc compiler sees I18n and TranslationPluralForms are both used in public documentation. We're having to suppress warnings about public-but-private things in typedoc.config.js

So it made sense to preserve all class, constructor and public function JSDoc comments

• do you think it'd be worth/possible to automate the addition of @preserve? I was thinking of something like adding it to any JSDoc block that doesn't have an @private using a Babel transform, but that likely depends on the rules for what needs that @preserve tag.

We don't have many, I'd probably keep it manual but your idea would work with { shouldPrintComment }

Pretty sure it keeps @license too by default

[colinrotherham]

@romaricpascal After we discussed @internal the other day I've updated this PR

I've taken your idea in #3958 (comment) to do things automatically:

1. Remove @internal comments
2. Remove @private comments
3. But keep @preserve comments

It's actually quite nice to see our descriptive developer comments preserved 😊

[colinrotherham]

But, here's another example with all comments removed except public JSDoc

--- A/packages/govuk-frontend/dist/govuk/components/character-count/character-count.mjs
+++ B/packages/govuk-frontend/dist/govuk/components/character-count/character-count.mjs
@@ -12,6 +12,8 @@
  *
  * You can configure the message to only appear after a certain percentage
  * of the available characters/words has been entered.
+ *
+ * @preserve
  */
 class CharacterCount {
   /**
@@ -19,40 +21,15 @@
    * @param {CharacterCountConfig} [config] - Character count config
    */
   constructor($module, config) {
-    /** @private */
     this.$module = void 0;
-    /** @private */
     this.$textarea = void 0;
-    /**
-     * @private
-     * @type {HTMLElement | null}
-     */
     this.$visibleCountMessage = null;
-    /**
-     * @private
-     * @type {HTMLElement | null}
-     */
     this.$screenReaderCountMessage = null;
-    /**
-     * @private
-     * @type {number | null}
-     */
     this.lastInputTimestamp = null;
-    /** @private */
     this.lastInputValue = '';
-    /**
-     * @private
-     * @type {number | null}
-     */
     this.valueChecker = null;
-    /**
-     * @private
-     * @type {CharacterCountConfig}
-     */
     this.config = void 0;
-    /** @private */
     this.i18n = void 0;
-    /** @private */
     this.maxLength = Infinity;
     if (!($module instanceof HTMLElement) || !document.body.classList.contains('govuk-frontend-supported')) {
       return this;
@@ -61,17 +38,7 @@
     if (!($textarea instanceof HTMLTextAreaElement || $textarea instanceof HTMLInputElement)) {
       return this;
     }
-
-    // Read config set using dataset ('data-' values)
     const datasetConfig = normaliseDataset($module.dataset);
-
-    // To ensure data-attributes take complete precedence, even if they change the
-    // type of count, we need to reset the `maxlength` and `maxwords` from the
-    // JavaScript config.
-    //
-    // We can't mutate `config`, though, as it may be shared across multiple
-    // components inside `initAll`.
-    /** @type {CharacterCountConfig} */
     let configOverrides = {};
     if ('maxwords' in datasetConfig || 'maxlength' in datasetConfig) {
       configOverrides = {
@@ -81,11 +48,8 @@
     }
     this.config = mergeConfigs(CharacterCount.defaults, config || {}, configOverrides, datasetConfig);
     this.i18n = new I18n(extractConfigByNamespace(this.config, 'i18n'), {
-      // Read the fallback if necessary rather than have it set in the defaults
       locale: closestAttributeValue($module, 'lang')
     });
-
-    // Determine the limit attribute (characters or words)
     if ('maxwords' in this.config && this.config.maxwords) {
       this.maxLength = this.config.maxwords;
     } else if ('maxlength' in this.config && this.config.maxlength) {
@@ -99,31 +63,17 @@
     if (!$textareaDescription) {
       return;
     }
-
-    // Inject a description for the textarea if none is present already
-    // for when the component was rendered with no maxlength, maxwords
-    // nor custom textareaDescriptionText
     if ($textareaDescription.innerText.match(/^\s*$/)) {
       $textareaDescription.innerText = this.i18n.t('textareaDescription', {
         count: this.maxLength
       });
     }
-
-    // Move the textarea description to be immediately after the textarea
-    // Kept for backwards compatibility
     this.$textarea.insertAdjacentElement('afterend', $textareaDescription);
-
-    // Create the *screen reader* specific live-updating counter
-    // This doesn't need any styling classes, as it is never visible
     const $screenReaderCountMessage = document.createElement('div');
     $screenReaderCountMessage.className = 'govuk-character-count__sr-status govuk-visually-hidden';
     $screenReaderCountMessage.setAttribute('aria-live', 'polite');
     this.$screenReaderCountMessage = $screenReaderCountMessage;
     $textareaDescription.insertAdjacentElement('afterend', $screenReaderCountMessage);
-
-    // Create our live-updating counter element, copying the classes from the
-    // textarea description for backwards compatibility as these may have been
-    // configured
     const $visibleCountMessage = document.createElement('div');
     $visibleCountMessage.className = $textareaDescription.className;
     $visibleCountMessage.classList.add('govuk-character-count__status');
@@ -130,69 +80,21 @@
     $visibleCountMessage.setAttribute('aria-hidden', 'true');
     this.$visibleCountMessage = $visibleCountMessage;
     $textareaDescription.insertAdjacentElement('afterend', $visibleCountMessage);
-
-    // Hide the textarea description
     $textareaDescription.classList.add('govuk-visually-hidden');
-
-    // Remove hard limit if set
     this.$textarea.removeAttribute('maxlength');
     this.bindChangeEvents();
-
-    // When the page is restored after navigating 'back' in some browsers the
-    // state of form controls is not restored until *after* the DOMContentLoaded
-    // event is fired, so we need to sync after the pageshow event.
     window.addEventListener('pageshow', () => this.updateCountMessage());
-
-    // Although we've set up handlers to sync state on the pageshow event, init
-    // could be called after those events have fired, for example if they are
-    // added to the page dynamically, so update now too.
     this.updateCountMessage();
   }
-
-  /**
-   * Bind change events
-   *
-   * Set up event listeners on the $textarea so that the count messages update
-   * when the user types.
-   *
-   * @private
-   */
   bindChangeEvents() {
     this.$textarea.addEventListener('keyup', () => this.handleKeyUp());
-
-    // Bind focus/blur events to start/stop polling
     this.$textarea.addEventListener('focus', () => this.handleFocus());
     this.$textarea.addEventListener('blur', () => this.handleBlur());
   }
-
-  /**
-   * Handle key up event
-   *
-   * Update the visible character counter and keep track of when the last update
-   * happened for each keypress
-   *
-   * @private
-   */
   handleKeyUp() {
     this.updateVisibleCountMessage();
     this.lastInputTimestamp = Date.now();
   }
-
-  /**
-   * Handle focus event
-   *
-   * Speech recognition software such as Dragon NaturallySpeaking will modify the
-   * fields by directly changing its `value`. These changes don't trigger events
-   * in JavaScript, so we need to poll to handle when and if they occur.
-   *
-   * Once the keyup event hasn't been detected for at least 1000 ms (1s), check if
-   * the textarea value has changed and update the count message if it has.
-   *
-   * This is so that the update triggered by the manual comparison doesn't
-   * conflict with debounced KeyboardEvent updates.
-   *
-   * @private
-   */
   handleFocus() {
     this.valueChecker = window.setInterval(() => {
       if (!this.lastInputTimestamp || Date.now() - 500 >= this.lastInputTimestamp) {
@@ -200,24 +102,9 @@
       }
     }, 1000);
   }
-
-  /**
-   * Handle blur event
-   *
-   * Stop checking the textarea value once the textarea no longer has focus
-   *
-   * @private
-   */
   handleBlur() {
-    // Cancel value checking on blur
     clearInterval(this.valueChecker);
   }
-
-  /**
-   * Update count message if textarea value has changed
-   *
-   * @private
-   */
   updateIfValueChanged() {
     if (this.$textarea.value !== this.lastInputValue) {
       this.lastInputValue = this.$textarea.value;
@@ -224,37 +111,17 @@
       this.updateCountMessage();
     }
   }
-
-  /**
-   * Update count message
-   *
-   * Helper function to update both the visible and screen reader-specific
-   * counters simultaneously (e.g. on init)
-   *
-   * @private
-   */
   updateCountMessage() {
     this.updateVisibleCountMessage();
     this.updateScreenReaderCountMessage();
   }
-
-  /**
-   * Update visible count message
-   *
-   * @private
-   */
   updateVisibleCountMessage() {
     const remainingNumber = this.maxLength - this.count(this.$textarea.value);
-
-    // If input is over the threshold, remove the disabled class which renders the
-    // counter invisible.
     if (this.isOverThreshold()) {
       this.$visibleCountMessage.classList.remove('govuk-character-count__message--disabled');
     } else {
       this.$visibleCountMessage.classList.add('govuk-character-count__message--disabled');
     }
-
-    // Update styles
     if (remainingNumber < 0) {
       this.$textarea.classList.add('govuk-textarea--error');
       this.$visibleCountMessage.classList.remove('govuk-hint');
@@ -264,26 +131,14 @@
       this.$visibleCountMessage.classList.remove('govuk-error-message');
       this.$visibleCountMessage.classList.add('govuk-hint');
     }
-
-    // Update message
     this.$visibleCountMessage.innerText = this.getCountMessage();
   }
-
-  /**
-   * Update screen reader count message
-   *
-   * @private
-   */
   updateScreenReaderCountMessage() {
-    // If over the threshold, remove the aria-hidden attribute, allowing screen
-    // readers to announce the content of the element.
     if (this.isOverThreshold()) {
       this.$screenReaderCountMessage.removeAttribute('aria-hidden');
     } else {
       this.$screenReaderCountMessage.setAttribute('aria-hidden', 'true');
     }
-
-    // Update message
     this.$screenReaderCountMessage.innerText = this.getCountMessage();
   }
 
@@ -297,7 +152,7 @@
    */
   count(text) {
     if ('maxwords' in this.config && this.config.maxwords) {
+      const tokens = text.match(/\S+/g) || [];
-      const tokens = text.match(/\S+/g) || []; // Matches consecutive non-whitespace chars
       return tokens.length;
     } else {
       return text.length;
@@ -347,26 +202,14 @@
    *   (or no threshold is set)
    */
   isOverThreshold() {
-    // No threshold means we're always above threshold so save some computation
     if (!this.config.threshold) {
       return true;
     }
-
-    // Determine the remaining number of characters/words
     const currentLength = this.count(this.$textarea.value);
     const maxLength = this.maxLength;
     const thresholdValue = maxLength * this.config.threshold / 100;
     return thresholdValue <= currentLength;
   }
-
-  /**
-   * Character count default config
-   *
-   * @see {@link CharacterCountConfig}
-   * @constant
-   * @default
-   * @type {CharacterCountConfig}
-   */
 }
 
 /**
@@ -455,7 +298,6 @@
 CharacterCount.defaults = Object.freeze({
   threshold: 0,
   i18n: {
-    // Characters
     charactersUnderLimit: {
       one: 'You have %{count} character remaining',
       other: 'You have %{count} characters remaining'
@@ -465,7 +307,6 @@
       one: 'You have %{count} character too many',
       other: 'You have %{count} characters too many'
     },
-    // Words
     wordsUnderLimit: {
       one: 'You have %{count} word remaining',
       other: 'You have %{count} words remaining'

[romaricpascal]

Just two little questions, one on the implementation of what picks what we remove, the other on the tagging of the methods of I18n. Other than that, keen to get the heavy reduction of size in our package 😊

[romaricpascal]

question: Would matching /** help us not have to add @preserve tags and limit the manual flagging of comments we want to keep (like all the classes JSDoc blocks)?

[colinrotherham]

Sometimes we just have non-documentation comments like:

/**
 * Hello Romaric
 */

So they get removed too

[romaricpascal]

Good shout, that wasn't super useful 🙌🏻

[romaricpascal]

question I think outside of t, I18n's methods are @private (to the class) rather than @internal (to our package). Does flagging them as internal help with something else (TypeScript maybe)?

[colinrotherham]

No not really, we'd just need to watch out for these ones:

1. t()
2. getPluralSuffix()
3. getPluralRulesForLocale()
4. selectPluralFormUsingFallbackRules()

We call them externally from tests so it's wrong to make them @private

[romaricpascal]

Is that something that TypeScript would flag? In which case we can go with the process of trying to lock things as @private, if TypeScript flags (for tests, for ex) @internal.

[romaricpascal]

In that respect, fine with the methods you list to stay @internal (as we need them to be for testing) 😊

[colinrotherham]

Sadly we also add mocks/spies to private methods:

jest.spyOn(i18n, 'hasIntlPluralRulesSupport')
  .mockImplementation(() => false)

I'll keep them all as @internal and we can assess how we test "non-public" class interfaces another day!

[romaricpascal]

Yeah, may be more efficient to go for @internal all the way + some convention (the commonplace leading _) for things that are meant to only be accessed from within the class. I've scheduled something in dev catch up to discuss the pains of marking things properly @private so we can chose knowingly whether to embrace it.

[romaricpascal]

That's quite a shave from removing the comments.

Ran some comparison on the package size¹ as well and we're shaving 14% by cleaning those comments, that's nice.

# main
npm notice name:          govuk-frontend                          
npm notice version:       4.6.0                                   
npm notice filename:      govuk-frontend-4.6.0.tgz                
npm notice package size:  886.2 kB                                
npm notice unpacked size: 4.3 MB                                  

# rollup-babel-comments
npm notice name:          govuk-frontend                          
npm notice version:       4.6.0                                   
npm notice filename:      govuk-frontend-4.6.0.tgz                
npm notice package size:  760.6 kB                                
npm notice unpacked size: 3.7 MB                                  

Footnotes

1. With npm ci then npm run build:package then npm pack --dry-run --workspace govuk-frontend on each branch ↩