Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit polyfill impact on bundle size to components that actually import them #2959

Merged
merged 3 commits into from
Nov 7, 2022

Conversation

romaricpascal
Copy link
Member

@romaricpascal romaricpascal commented Nov 2, 2022

This PR extracts the functions that use polyfills in their own modules, allowing components to import only the polyfills that they will use:

  • normaliseDataset which uses String.prototype.trim
  • closestAttributeValue which uses Element.prototype.closest
  • getDataset (introduced a function to make a central point for loading the polyfill for Element.prototype.dataset)

Currently, these methods are in common.mjs (or not exist at all for getDataset). This means that if some code imported individual components which import common.mjs for other helpers (like the Checkboxes component), the bundled code would get the 3 aforementionned polyfills.

[This spreasheet shows a comparion of file sizes before and after extraction] (main being 506e4f051 at the time of the checks). It shows the difference in the modules bundled from imporing each component, as well as the weight of:

  • the aggregated raw files (if I understood correctly what stats meant from webpack-bundle-analyzer)
  • the minified files (resulting of the build, and which the browser will need parsing)
  • the gziped file (probably least relevant, as that number will be affected by the rest of the code using the component)

Methodology is explained in the footnotes1.

As a result of the extraction:

  • polyfill modules are no longer bundled with components that don't need them 🎉
  • there is no significant increase of bundle size for all.js: aggregated size does increase, but gets minified to the same weight 🎉
  • we're only gaining ~1.85Kb of minified JS for modules that inadvertently got all polyfills, or 0.8 for those that only didn't need String.protopype.trim. It's a small win (but still a win?).

Closes #2907

Footnotes

  1. Methodology

    1. Each individual component files, as well as all.mjs was bundled with Webpack (as it's the most popular bundler in the frontend survey), exporting the built file and bundling stats. This simulates a 3rd party project importing the module in their code and Webpack following the trail of imports to add whatever needs adding to the bundled output.
    2. webpack-bundle-analyzer was run on each stats file to get a visual report of the modules bundled for each build and their sizes, which were then reported in the spreadsheet.

    This was automated by the following script, run on main (506e4f051 at the time) and the head of this branch.

    #!/bin/sh
    
    # Grab a unit reference for the Git commit based on the path from the `main`` branch
    # but dropping the `heads/` (a bit bluntly, but basename does the job)
    REF=`basename $(git describe --all --match=main)`
    echo $REF
    
    # Bundles the file passed as arguments, collecting some stats and generating a report
    # with webpack-bundle-analyzer
    bundle_file () {
      # A bit clearer to read than dragging $1 (first argument) to reference the filename
      file_path=$1
    
      # Reference the build name (component name or 'all') from the file name
      build_name=$(basename $file_path .mjs)
      # Create a folder to store output in a tidy fashion
      output_path="tmp/$REF/$build_name"
      mkdir -p $output_path
    
      # 1. Bundle the component file and export JSON stats
      npx webpack --mode=production "./$file_path" -o "$output_path" --json > "$output_path/stats.json"
      # 2. Generate a report using webpack-bundle-analyzer so we can visually see what's changed
      #    https://github.com/webpack-contrib/webpack-bundle-analyzer#usage-as-a-cli-utility
      (cd $output_path && npx webpack-bundle-analyzer --mode=static -O "stats.json" ".")
    }
    
    # Build the overall package to check impact when all components are bundled together
    # Adding more modules means Webpack will wrap each of them in some boilerplate code
    # for properly isolating them, as well as add some code to handle exports and import
    bundle_file src/govuk/all.mjs
    
    # Build each component file. This simulates what Webpack would bundle if the component
    # was imported on its own by a project
    for componentJavaScriptFile in src/govuk/components/**/*.mjs; do
      bundle_file $componentJavaScriptFile
    done
    

    As a side note, I'd be keen to see if we could automate that kind of bundle content collection in a Github action as a safety for inadvertently adding polyfills 😄

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2959 November 2, 2022 18:47 Inactive
@romaricpascal romaricpascal marked this pull request as ready for review November 3, 2022 18:33
@romaricpascal romaricpascal requested a review from a team November 3, 2022 18:34
@romaricpascal
Copy link
Member Author

If we'd want to be even more narrow, we could create functional wrappers1 of the polyfilled methods, similarly to what getDataset does:

import '../vendor/polyfills/Element/prototype/closest.mjs

export function closest(element, selector) {
  return element.closest(selector);
}
import '../vendor/polyfills/String/prototype/trim.mjs

export function trim(value) {
  return value.trim()
}

This would give a very clear entry point for each polyfill that we'd have to use instead of calling the method on the object.
With the limited amount of methods affected, 5.0 around the corner and the limited gain after update, I figured we could be a bit more coarse, but happy to revisit if we feel it's a good idea.

Footnotes

  1. Not quite ponyfills, but kind-of

@colinrotherham
Copy link
Contributor

colinrotherham commented Nov 4, 2022

Thanks @romaricpascal this looks good

Removes the "side effects" from common.mjs for #2907 🎉

Only thing I'd (probably) suggest is to rename "polyfilled" to "common" (or "utilities") and keep all our common utilities split up into separate files—but all together. Doesn't necessarily help to group polyfilled ones separately?

src/govuk
├── assets
├── common
│   ├── closest-attribute-value.mjs
│   ├── closest-attribute-value.unit.test.mjs
│   ├── get-dataset.mjs
│   ├── get-dataset.unit.test.mjs
│   ├── index.mjs
│   ├── normalise-dataset.mjs
│   ├── normalise-dataset.unit.test.mjs
│   └── …etc…
├── common.mjs
└── …etc…

For backwards compatibility I've left the original common.mjs file with only a namespace export

src/govuk/common.mjs

export * from './common/index.mjs'

Which re-exports from the common directory index:

src/govuk/common/index.mjs

export { closestAttributeValue } from './closest-attribute-value.mjs'
export { getDataset } from './get-dataset.mjs'
export { normaliseDataset } from './normalise-dataset.mjs'

Think we should also stick to:

  1. Using kebab-case.js for file names
  2. Using *.unit.test.mjs (ESM) as the default for unit tests

(Don't want to put too much thought into polyfill structure as we may nearly be ready to import what we need from core-js, or using @babel/preset-env to add them automatically, instead of maintaining our own)

@romaricpascal
Copy link
Member Author

Good shout on putting everything into a "commons" folder rather than my weird "polyfilled"! I'll update and tidy up the names of the files and tests.

This'll allow the polyfill it requires to be tree-shaken properly when bundling components
that require common.mjs
Allows the polyfill it requires to be tree-shaken properly
when components that don't use it import `common.mjs`
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2959 November 7, 2022 14:48 Inactive
* @param {nodeListIterator} callback - Callback function to run for each node
* @returns {undefined}
*/
export function nodeListForEach (nodes, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers for reorganising this 😊

Thoughts on making the index.mjs file a namespace export? Typically we'd want index.mjs to export everything from the directory

Currently two imports are needed:

import { closestAttributeValue } from './common/closest-attribute-value.mjs'
import { nodeListForEach } from './common/index.mjs'

Which we could turn into one?

import { closestAttributeValue, nodeListForEach } from './common/index.mjs'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers for chatting through this

Yep, we'd be bringing back "side effects" by importing all those polyfills again

Feels like we could import a "ready to run" polyfill but then we're back down the route of doing what core-js will do for us:

import { detect, polyfill } '../../vendor/polyfills/Element/prototype/closest.mjs'

if (!detect()) {
  polyfill()
}

I'm happy to leave this file as it is

* This is a functional wrapper around `element.dataset` to centralise the
* import of the dataset polyfill
*
* @param {HTMLElement|SVGElement} element - The element whose dataset to access
Copy link
Contributor

@colinrotherham colinrotherham Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to add SVGElement here but I have a feeling we might see conflicting types downstream, especially with document.activeElement returning the base Element

Can I suggest we leave out getDataset() from this PR and stick to reorganising only?

Leaves the `common.mjs` file to re-export it so that code importing it from the current release still find it
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2959 November 7, 2022 16:50 Inactive
@romaricpascal romaricpascal merged commit 9c32b47 into main Nov 7, 2022
@romaricpascal romaricpascal deleted the polyfill-tree-shaking branch November 7, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit side effects of polyfills loaded for config/I18n
3 participants