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

Rework variants to reduce bundle size #83

Merged
merged 39 commits into from
Sep 6, 2021
Merged

Conversation

MattiasBuelens
Copy link
Owner

@MattiasBuelens MattiasBuelens commented Jul 21, 2021

This removes some variants and build artifacts to reduce the total download size of the npm package.

  • Remove all generated source maps.
  • Remove the ES2018 variant, which is now combined into the ES6 variant. This means that ReadableStream.prototype[Symbol.asyncIterator] won't correctly inherit from %AsyncIteratorPrototype%, but nobody cares about that. 😛
  • Remove unminified polyfill variants. All build artifacts for the polyfill variants are now always minified, while the ponyfill variants are always unminified.

Fixes #82.

To do:

@MattiasBuelens
Copy link
Owner Author

With the current changes, we're at 182.0 kB packed, 1.1 MB unpacked. Still quite heavy...

npm pack
npm notice
npm notice package: [email protected]
npm notice === Tarball Contents ===
npm notice 1.1kB   LICENSE
npm notice 56.0kB  dist/polyfill.es6.min.js
npm notice 60.2kB  dist/polyfill.min.js
npm notice 187.5kB dist/ponyfill.es6.js
npm notice 198.1kB dist/ponyfill.js
npm notice 212B    es6/package.json
npm notice 2.4kB   package.json
npm notice 169B    ponyfill/es6/package.json
npm notice 148B    ponyfill/package.json
npm notice 329B    dist/types/tsdoc-metadata.json
npm notice 9.5kB   CHANGELOG.md
npm notice 7.0kB   README.md
npm notice 55.3kB  dist/polyfill.es6.min.mjs
npm notice 59.9kB  dist/polyfill.min.mjs
npm notice 170.2kB dist/ponyfill.es6.mjs
npm notice 180.2kB dist/ponyfill.mjs
npm notice 32.1kB  dist/types/polyfill.d.ts
npm notice 32.0kB  dist/types/ts3.6/polyfill.d.ts
npm notice === Tarball Details ===
npm notice name:          web-streams-polyfill
npm notice version:       3.0.3
npm notice filename:      web-streams-polyfill-3.0.3.tgz
npm notice package size:  182.0 kB
npm notice unpacked size: 1.1 MB
npm notice shasum:        31267b84b04aba59c868428e3a1fd5a216e105a7
npm notice integrity:     sha512-BVuy0GUcG+UCO[...]qtE78dlHSIY6Q==
npm notice total files:   18

@jimmywarting
Copy link

fyi, renaming, moving or removing different files/variants can cause a breaking change and would need a major change as ppl can depend on choosing their own variant to do the job

@MattiasBuelens
Copy link
Owner Author

I know, that's why I added a "semver-major" label. 😁

@MattiasBuelens MattiasBuelens changed the base branch from master to next July 21, 2021 21:59
@MattiasBuelens
Copy link
Owner Author

MattiasBuelens commented Aug 10, 2021

Down to 119 KB packaged, 638.8 KB unpacked.

I'm now using code splitting on the ESM bundles, so polyfill[.es6].mjs imports directly from ponyfill[.es6].mjs. Unfortunately, I can't do this for the UMD bundles.

npm pack
npm notice
npm notice package: [email protected]
npm notice === Tarball Contents ===
npm notice 1.1kB   LICENSE
npm notice 56.2kB  dist/polyfill.es6.js
npm notice 60.4kB  dist/polyfill.js
npm notice 55.3kB  dist/ponyfill.es6.js
npm notice 59.7kB  dist/ponyfill.js
npm notice 200B    es6/package.json
npm notice 2.2kB   package.json
npm notice 169B    ponyfill/es6/package.json
npm notice 148B    ponyfill/package.json
npm notice 329B    dist/types/tsdoc-metadata.json
npm notice 9.5kB   CHANGELOG.md
npm notice 7.0kB   README.md
npm notice 2.1kB   dist/polyfill.es6.mjs
npm notice 2.4kB   dist/polyfill.mjs
npm notice 170.0kB dist/ponyfill.es6.mjs
npm notice 180.1kB dist/ponyfill.mjs
npm notice 32.0kB  dist/types/polyfill.d.ts
npm notice === Tarball Details ===
npm notice name:          web-streams-polyfill
npm notice version:       3.1.0
npm notice filename:      web-streams-polyfill-3.1.0.tgz
npm notice package size:  119.0 kB
npm notice unpacked size: 638.8 kB
npm notice shasum:        c3cfb0dba11b916c0134f8cf765548f2d978f7db
npm notice integrity:     sha512-PDktomxray+Oh[...]ZOr+EkDKujVVg==
npm notice total files:   17

@jimmywarting
Copy link

jimmywarting commented Aug 11, 2021

could the polyfill load the ponyfill instead?

Meaning ponyfill would just be as tiny as loading the ponyfill and assigning it to the global namespace...

@wojpawlik
Copy link

No it can't. Polyfill would need to synchronously import the ponyfill.

It'd be neat if a single UMD build could polyfill when loaded in browser, and ponyfill when loaded as CJS.

Windows 11 won't ship IE. Are ES5 builds really needed?

This is looking pretty good already.

@wojpawlik
Copy link

  • Would it make sense to have ponyfill.mjs import from ponyfill.js

Probably not. It wouldn't work in browsers. And Node.js ^12.20.0 || >=14.13.1 can already detect named exports in CJS (haven't tested UMD): nodejs/node#35249 (comment).

@jimmywarting
Copy link

Windows 11 won't ship IE. Are ES5 builds really needed?

IE11 is pretty much dead already... down at 0.57% https://gs.statcounter.com


This is looking pretty good already.

Agree, but still think there is room for improvements, but don't know how. How about minifying all files?

@wojpawlik
Copy link

npm notice 200B    es6/package.json
npm notice 169B    ponyfill/es6/package.json
npm notice 148B    ponyfill/package.json

could be replaced with https://nodejs.org/api/packages.html#packages_subpath_exports.

And CHANGELOG.md could be .npmignored.

@MattiasBuelens
Copy link
Owner Author

Thanks for the suggestions! 🙂

Indeed, I cannot have the UMD polyfill load the UMD ponyfill like I did with ESM, because there's no general/reliable way to do that.

I don't know if people expect ESM bundles to be human readable? Anyway, I can turn on minification unconditionally.

The company I work for also uses this polyfill, and we still support IE11... 😬 I'll check if we can make the polyfill ES6-only and set up some transpilation on our node_modules.

I've got an experiment branch that uses package entry points instead. I want to first check if bundlers like Webpack and Rollup handle these correctly before I commit to it.

@jimmywarting
Copy link

jimmywarting commented Aug 15, 2021

I don't know if people expect ESM bundles to be human readable

Doesn't matter to me so much. At least when it comes to spec'ed apis
I mostly expect them to say the function toString is native code. And I know what the code is suppose to do/output. I don't go so much into depths on built-in codes. Polyfills should not be tampered with anyways.
Much less so when typing exist to help out

Other packages yes. Then it is nice with readable code.

Minified code loads/parse faster and and have the potential for code optimization that makes it run faster.

So I'm +1 for minification

@jimmywarting
Copy link

Just speculation. But could the typing reference (or extend) the Dom implementation so d.ts could be reduced?

@MattiasBuelens
Copy link
Owner Author

MattiasBuelens commented Aug 15, 2021

And CHANGELOG.md could be .npmignored.

Apparently that's not possible. From the npm docs:

The following paths and files are never ignored, so adding them to .npmignore is pointless:

  • package.json
  • README (and its variants)
  • CHANGELOG (and its variants)
  • LICENSE / LICENCE

So CHANGELOG.md is always included in the package.


Just speculation. But could the typing reference (or extend) the Dom implementation so d.ts could be reduced?

That requires a /// <reference lib="dom" /> which is likely to cause issues for Node users, see microsoft/TypeScript#33111... 😬 It looks like a similar remark was raised in a recent change to node-fetch? Not sure if/how it was tackled there.

Looks like they're trying to tackle this by extracting the type definitions shared between Node and DOM into a separate file: microsoft/TypeScript#43972. That would solve the problem.

@jimmywarting
Copy link

Working a bit with deno nowdays... guess full urls don't work with microsofts own typescript yet

/// <reference lib="https://github.com/MattiasBuelens/web-streams-polyfill/blob/36c08de41aa3a5699afab717b0e64af13dada56f/x.d.ts" />

the alternative is hosting types at @types

@MattiasBuelens
Copy link
Owner Author

Minification is now enabled across the board. We're now at 87.0 kB packed, 401.8 kB unpacked.

npm pack
npm notice
npm notice package: [email protected]
npm notice === Tarball Contents ===
npm notice 1.1kB  LICENSE
npm notice 56.2kB dist/polyfill.es6.js
npm notice 60.4kB dist/polyfill.js
npm notice 55.3kB dist/ponyfill.es6.js
npm notice 59.7kB dist/ponyfill.js
npm notice 2.9kB  package.json
npm notice 329B   dist/types/tsdoc-metadata.json
npm notice 9.5kB  CHANGELOG.md
npm notice 7.0kB  README.md
npm notice 1.6kB  dist/polyfill.es6.mjs
npm notice 1.6kB  dist/polyfill.mjs
npm notice 54.6kB dist/ponyfill.es6.mjs
npm notice 59.5kB dist/ponyfill.mjs
npm notice 32.0kB dist/types/polyfill.d.ts
npm notice === Tarball Details ===
npm notice name:          web-streams-polyfill
npm notice version:       3.1.0
npm notice filename:      web-streams-polyfill-3.1.0.tgz
npm notice package size:  87.0 kB
npm notice unpacked size: 401.8 kB
npm notice shasum:        b0dbafc956733f5b64e8763077c326adb8039191
npm notice integrity:     sha512-8zEDoB88N8ZXM[...]Fgu/9aDufuvrg==
npm notice total files:   14

Package entry points seem to work fine in the latest versions of Rollup and Webpack, so I made the switch. For my own sanity, I've added tests for both bundlers and for require-ing it directly with Node. 😁

package.json Outdated Show resolved Hide resolved
@wojpawlik
Copy link

What do dist/ponyfill.es6.js and dist/ponyfill.js do when loaded in a browser anyway?

package.json Outdated Show resolved Hide resolved
@MattiasBuelens
Copy link
Owner Author

What do dist/ponyfill.es6.js and dist/ponyfill.js do when loaded in a browser anyway?

They can register with an AMD module loader (like RequireJS through define()). If no such module loader is found, they register a single global variable called WebStreamsPolyfill (defined here), so you can access the exported classes with new WebStreamsPolyfill.ReadableStream().

Do I expect anyone to actually be using this? Not really... 😛

@MattiasBuelens MattiasBuelens changed the title Reduce bundle size Rework variants to reduce bundle size Aug 18, 2021
@MattiasBuelens
Copy link
Owner Author

I made the ES6 ponyfill the default variant. If we're going to make breaking changes, we're going to really break things. 😛

Still not sure about the name of the polyfill variant though: import "web-streams-polyfill/polyfill" sounds... repetitive. Suggestions welcome!

There's not much of a difference in the package size: 86.2 kB packed, 397.0 kB unpacked.

npm pack
npm notice package: [email protected]
npm notice === Tarball Contents ===
npm notice 1.1kB  LICENSE
npm notice 59.7kB dist/polyfill.es5.js
npm notice 55.2kB dist/polyfill.js
npm notice 59.7kB dist/ponyfill.es5.js
npm notice 55.3kB dist/ponyfill.js
npm notice 2.8kB  package.json
npm notice 329B   dist/types/tsdoc-metadata.json
npm notice 9.5kB  CHANGELOG.md
npm notice 7.2kB  README.md
npm notice 59.5kB dist/ponyfill.es5.mjs
npm notice 54.6kB dist/ponyfill.mjs
npm notice 32.0kB dist/types/ponyfill.d.ts
npm notice === Tarball Details ===
npm notice name:          web-streams-polyfill
npm notice version:       3.1.0
npm notice filename:      web-streams-polyfill-3.1.0.tgz
npm notice package size:  86.2 kB
npm notice unpacked size: 397.0 kB
npm notice shasum:        debf2c6af0fb5115af08b9caa8ecdc8f4acee598
npm notice integrity:     sha512-tkJkzQcEgeVXW[...]NU+H64Wv0ibsg==
npm notice total files:   12

package.json Outdated Show resolved Hide resolved
@MattiasBuelens
Copy link
Owner Author

Hmm, now that the polyfill variant does not export anything, I suppose it needs a different .d.ts to reflect that... It also looks like TypeScript might not yet support package entry points, see microsoft/TypeScript#33079. 😕

@jimmywarting jimmywarting mentioned this pull request Aug 23, 2021
35 tasks
@MattiasBuelens
Copy link
Owner Author

Found a workaround for the type definitions using typesVersions. Now, import "web-streams-polyfill/polyfill" will have no exports, to match the run-time behavior. 😁

Even better: it will now augment TypeScript's built-in ReadableStream type with the new methods added by the polyfill. Right now, that's getReader({ mode: 'byob' }) for BYOB readers and values() for async iteration.

I would have liked to make new ReadableStream({ type: 'bytes' }) work too, but I don't think that's possible with how lib.dom.d.ts defines the global ReadableStream variable. 😞

Still not sure about the name of that entry point though. I've come up with a few alternatives:

  • import "web-streams-polyfill/install", to indicate that this will install the polyfill in the global scope
  • import "web-streams-polyfill/register", inspired by @babel/register and ts-node/register

@MattiasBuelens
Copy link
Owner Author

Released version 4.0.0-beta.1, see release notes. Looking forward to your feedback! 😀

@MattiasBuelens MattiasBuelens deleted the reduce-bundle-size branch September 6, 2021 20:21
@jimmywarting
Copy link

jimmywarting commented Sep 6, 2021

Cool! Want to test it now, but guess i will do it tomorrow instead (22:23) here now.
+ i'm busy with another project i'm working on right now

@jimmywarting
Copy link

402 kB, That is a good reduction :)
(compared to prev 7.56 MB)

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.

Ready to boost your popularity to like 22 mil download / week?
3 participants