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

Extract dedicated polyfill package #6060

Merged
merged 7 commits into from
Mar 15, 2022
Merged

Extract dedicated polyfill package #6060

merged 7 commits into from
Mar 15, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 14, 2022

Context: #6023 (comment)

Why:

  • So that a majority of users don't have to pay a performance penalty associated with supporting legacy browsers.
  • Simplify logic for initialization by setting expectation of feature availability
    • And, consequently, as an incremental step toward dropping polyfill/IE11 support altogether
  • Reduce size of bundles by excluding more polyfills injected by core-js (notably, Promise and URL polyfills)

**Why**:

- So that a majority of users don't have to pay a performance penalty associated with supporting legacy browsers.
- Simplify logic for initialization by setting expectation of feature availability
   - And, consequently, as an incremental step toward dropping polyfill/IE11 support altogether
- Reduce size of bundles by excluding more polyfills injected by core-js (notably, Promise and URL polyfills)
@aduth aduth marked this pull request as draft March 14, 2022 18:29
@@ -8,6 +8,7 @@
"clipboard-polyfill": "^3.0.3",
"custom-event-polyfill": "^1.0.7",
"js-polyfills": "^0.1.43",
"promise-polyfill": "^8.2.3",
Copy link
Member Author

Choose a reason for hiding this comment

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

Promises were only polyfilled previously because we use them in our own code, via @babel/preset-env's useBuiltIns: 'usage' option. By default, third-party packages are excluded from Babel transforms. Since some of the polyfills themselves rely on the Promise polyfill (fetch, webcrypto-shim), it needs to be explicitly included.

@@ -61,7 +61,7 @@ module.exports = /** @type {import('webpack').Configuration} */ ({
},
optimization: {
chunkIds: 'natural',
splitChunks: { chunks: 'all' },
splitChunks: { chunks: (chunk) => chunk.name !== 'polyfill' },
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for this is to ensure that polyfill is kept as a single, self-contained pack, since we don't (and cannot reasonably) share default script chunk rendering between the polyfill and other scripts, due to the need for nomodule attribute on the polyfill script.

Restore initializer function, exclude from test side-effect via similar pattern as webauthn-unhide script
@aduth
Copy link
Member Author

aduth commented Mar 14, 2022

Based on some rough measuring, seems like this has a pretty nice improvement for script size sent for modern browsers, down ~10% (90.3kb to 81kb) for the sign in page.

Thinking this is due to a combination of:

@aduth aduth marked this pull request as ready for review March 14, 2022 21:00
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM!

Avoid additional code and overhead related to dynamic imports

Would it be worth adding a lint to disallow dynamic imports to help keep us like this in the future? Or should we just trust our team to add things as needed?

@aduth
Copy link
Member Author

aduth commented Mar 14, 2022

Would it be worth adding a lint to disallow dynamic imports to help keep us like this in the future? Or should we just trust our team to add things as needed?

I think there could still be some valid use-cases for dynamic imports, such as importing some heavy JavaScript in specific conditions / events. The overhead is probably not too much. It's mostly realizing that each dynamic import causes a chunk to be created, and each chunk carries with it a small amount of base cost.

Here, I'm guessing it's the polyfill exclusions that have the biggest impact on size reduction. In the future, we can probably get more aggressive with this once we can target newer browsers via @babel/preset-env. We could even consider the approach from the aforementioned article, creating two separate sets of scripts for legacy and modern browsers.

**Why**: Since it won't be loaded by modern browsers, and since those modern browsers like to complain about preload header not followed by actual load of the script
@aduth
Copy link
Member Author

aduth commented Mar 15, 2022

Here, I'm guessing it's the polyfill exclusions that have the biggest impact on size reduction. In the future, we can probably get more aggressive with this once we can target newer browsers via @babel/preset-env. We could even consider the approach from the aforementioned article, creating two separate sets of scripts for legacy and modern browsers.

A more realistic short-term option could be to disable all Babel polyfilling (useBuiltIns: false, docs), and import the entire core-js package into @18f/identity-polyfill.

@aduth aduth merged commit 07a0a17 into main Mar 15, 2022
@aduth aduth deleted the aduth-polyfill-all branch March 15, 2022 15:20
aduth added a commit that referenced this pull request Apr 12, 2022
**Why**:

- Because polyfills are intended to be confined to the polyfill package (#6060)
- So that it's not included in modern browsers (#6068, -0.9kb gzipped)
- To reduce surface area of dependencies shared with USWDS (related to 18F/identity-design-system#312 (comment))

changelog: Internal, Optimization, Reduce size of JavaScript for modern browsers
aduth added a commit that referenced this pull request Apr 13, 2022
**Why**:

- Because polyfills are intended to be confined to the polyfill package (#6060)
- So that it's not included in modern browsers (#6068, -0.9kb gzipped)
- To reduce surface area of dependencies shared with USWDS (related to 18F/identity-design-system#312 (comment))

changelog: Internal, Optimization, Reduce size of JavaScript for modern browsers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants