Bundle size query (and 8kB version)

dracos commented 1 month ago

Thank you for continuing to maintain this repository. Following on from the similar #321, it looks like v3 increases the size of this package from 11.3kB minified+gzipped (or 8kB, if you tweaked the configuration as in my comment on that issue) to 18.4kB : .

Looking at a comparison of the actual source change between v2.0.4 and v3.0.0, with e.g. git diff v2.0.4..39a52dceb0 src/autocomplete.js (all the other src/ changes are small), there doesn't appear to be anything in there that would explain such a large increase, especially as you say you're also dropping old-IE polyfills. There's only the changes you outline in the release notes (plus #676 which appears to be missing, hope that's helpful).

I have applied the PRs (bar the additional classes which didn't cherry-pick cleanly in the time I had available) on top of my fork, it's still only 8kB and seems to work fine: https://github.com/mysociety/accessible-autocomplete/tree/v2.0.4-ms2

Diffing src shows the missing "additional classes" code, but that's all; diffing dist shows the expected changes - but the v3.0.0 file has an awful lot of extra code up front. What it appears to be doing is including a large number of core-js polyfills, e.g. Array.prototype.push (and all its internal dependents) allegedly in order to deal with the esoteric edge case of https://stackoverflow.com/a/77556853/669631. I'm unclear as to the purpose of including these polyfills in a library like this (does it really need to include a polyfill for Array.prototype.join?), that almost certainly doesn't need any of them, and more than doubles the size of the gzipped library (never mind the ungzipped). I wonder if there's a way to tweak the configuration so as not to include them?

romaricpascal commented 1 month ago

Thanks for flagging this and digging into this! We've had a round of updating our packages and Babel configuration as part of matching the browsers we declared to support and more generally keeping our tools up to date. This changed the polyfills we're including (from core-js 2 to core-js 3, as well as an update of our .browserslistrc), so I'd wager that's where the extra burden comes from.

Looks like we need a deeper look at what's being brought in and which options we have for reducing included polyfills, as ideally we'd want only the polyfill code that we'd actively be using and not that for edge cases we'd never encounter.

There's still a chance some or all of these polyfills may remain needed, as being compatible with the browsers we want to be compatible with requires both to:

transform the syntax from what we author to what they support
and fill-in for missing features with polyfills.

dracos commented 1 month ago

Yeah, I understand the reasons - hopefully I can do your deeper look for you here. The current min JS is 54868 bytes. If I comment out useBuiltIns: 'usage' from your babel.config.js, and run npm run build, the JS is 23080 bytes (42%). The file is still ES5-compatible (according to es5-validator), preact doesn't require any polyfills for IE11, and your code doesn't use anything that requires this type of polyfilling in any way. Babel already compiles to ES5 by default in the absence of any specific targets, and that is a wider pool than your targeted browsers (this is also what my fork is doing). I don't think there are any issues introduced by this, though am happy to be corrected. If that's the only change that's needed, can I please suggest it's turned off? :-)

I find it funny that https://babeljs.io/docs/options#targets states "We recommend setting targets to reduce the output code size" when I have only ever found it do the opposite. I see this sort of thing has been raised in core-js at e.g. https://github.com/zloirock/core-js/issues/388 or on Reddit at https://www.reddit.com/r/javascript/comments/n5eqie/askjs_is_it_just_me_or_is_corejs_fundamentally/ - core-js always includes maximal IE8- support, even though you've said you don't support that. v4 of core-js will apparently drop that, so that might make a difference, but I still don't think it's at all necessary here.

As one example, your code uses Array.prototype.join, which has been in browsers for ever. Because of this, the following dependency tree is added to the distribution:

madge dependency tree on es.array.join.js

es.array.join.js
  ../internals/array-method-is-strict.js
  ../internals/export.js
  ../internals/function-uncurry-this.js
  ../internals/indexed-object.js
  ../internals/to-indexed-object.js
../internals/a-callable.js
  ../internals/is-callable.js
  ../internals/try-to-string.js
../internals/an-object.js
  ../internals/is-object.js
../internals/array-includes.js
  ../internals/length-of-array-like.js
  ../internals/to-absolute-index.js
  ../internals/to-indexed-object.js
../internals/array-method-is-strict.js
  ../internals/fails.js
../internals/classof-raw.js
  ../internals/function-uncurry-this.js
../internals/copy-constructor-properties.js
  ../internals/has-own-property.js
  ../internals/object-define-property.js
  ../internals/object-get-own-property-descriptor.js
  ../internals/own-keys.js
../internals/create-non-enumerable-property.js
  ../internals/create-property-descriptor.js
  ../internals/descriptors.js
  ../internals/object-define-property.js
../internals/create-property-descriptor.js
../internals/define-built-in.js
  ../internals/define-global-property.js
  ../internals/is-callable.js
  ../internals/make-built-in.js
  ../internals/object-define-property.js
../internals/define-global-property.js
  ../internals/global.js
../internals/descriptors.js
  ../internals/fails.js
../internals/document-create-element.js
  ../internals/global.js
  ../internals/is-object.js
../internals/engine-user-agent.js
../internals/engine-v8-version.js
  ../internals/engine-user-agent.js
  ../internals/global.js
../internals/enum-bug-keys.js
../internals/export.js
  ../internals/copy-constructor-properties.js
  ../internals/create-non-enumerable-property.js
  ../internals/define-built-in.js
  ../internals/define-global-property.js
  ../internals/global.js
  ../internals/is-forced.js
  ../internals/object-get-own-property-descriptor.js
../internals/fails.js
../internals/function-bind-native.js
  ../internals/fails.js
../internals/function-call.js
  ../internals/function-bind-native.js
../internals/function-name.js
  ../internals/descriptors.js
  ../internals/has-own-property.js
../internals/function-uncurry-this.js
  ../internals/function-bind-native.js
../internals/get-built-in.js
  ../internals/global.js
  ../internals/is-callable.js
../internals/get-method.js
  ../internals/a-callable.js
  ../internals/is-null-or-undefined.js
../internals/global.js
../internals/has-own-property.js
  ../internals/function-uncurry-this.js
  ../internals/to-object.js
../internals/hidden-keys.js
../internals/ie8-dom-define.js
  ../internals/descriptors.js
  ../internals/document-create-element.js
  ../internals/fails.js
../internals/indexed-object.js
  ../internals/classof-raw.js
  ../internals/fails.js
  ../internals/function-uncurry-this.js
../internals/inspect-source.js
  ../internals/function-uncurry-this.js
  ../internals/is-callable.js
  ../internals/shared-store.js
../internals/internal-state.js
  ../internals/create-non-enumerable-property.js
  ../internals/global.js
  ../internals/has-own-property.js
  ../internals/hidden-keys.js
  ../internals/is-object.js
  ../internals/shared-key.js
  ../internals/shared-store.js
  ../internals/weak-map-basic-detection.js
../internals/is-callable.js
../internals/is-forced.js
  ../internals/fails.js
  ../internals/is-callable.js
../internals/is-null-or-undefined.js
../internals/is-object.js
  ../internals/is-callable.js
../internals/is-pure.js
../internals/is-symbol.js
  ../internals/get-built-in.js
  ../internals/is-callable.js
  ../internals/object-is-prototype-of.js
  ../internals/use-symbol-as-uid.js
../internals/length-of-array-like.js
  ../internals/to-length.js
../internals/make-built-in.js
  ../internals/descriptors.js
  ../internals/fails.js
  ../internals/function-name.js
  ../internals/function-uncurry-this.js
  ../internals/has-own-property.js
  ../internals/inspect-source.js
  ../internals/internal-state.js
  ../internals/is-callable.js
../internals/math-trunc.js
../internals/object-define-property.js
  ../internals/an-object.js
  ../internals/descriptors.js
  ../internals/ie8-dom-define.js
  ../internals/to-property-key.js
  ../internals/v8-prototype-define-bug.js
../internals/object-get-own-property-descriptor.js
  ../internals/create-property-descriptor.js
  ../internals/descriptors.js
  ../internals/function-call.js
  ../internals/has-own-property.js
  ../internals/ie8-dom-define.js
  ../internals/object-property-is-enumerable.js
  ../internals/to-indexed-object.js
  ../internals/to-property-key.js
../internals/object-get-own-property-names.js
  ../internals/enum-bug-keys.js
  ../internals/object-keys-internal.js
../internals/object-get-own-property-symbols.js
../internals/object-is-prototype-of.js
  ../internals/function-uncurry-this.js
../internals/object-keys-internal.js
  ../internals/array-includes.js
  ../internals/function-uncurry-this.js
  ../internals/has-own-property.js
  ../internals/hidden-keys.js
  ../internals/to-indexed-object.js
../internals/object-property-is-enumerable.js
../internals/ordinary-to-primitive.js
  ../internals/function-call.js
  ../internals/is-callable.js
  ../internals/is-object.js
../internals/own-keys.js
  ../internals/an-object.js
  ../internals/function-uncurry-this.js
  ../internals/get-built-in.js
  ../internals/object-get-own-property-names.js
  ../internals/object-get-own-property-symbols.js
../internals/require-object-coercible.js
  ../internals/is-null-or-undefined.js
../internals/shared-key.js
  ../internals/shared.js
  ../internals/uid.js
../internals/shared-store.js
  ../internals/define-global-property.js
  ../internals/global.js
  ../internals/is-pure.js
../internals/shared.js
  ../internals/shared-store.js
../internals/symbol-constructor-detection.js
  ../internals/engine-v8-version.js
  ../internals/fails.js
  ../internals/global.js
../internals/to-absolute-index.js
  ../internals/to-integer-or-infinity.js
../internals/to-indexed-object.js
  ../internals/indexed-object.js
  ../internals/require-object-coercible.js
../internals/to-integer-or-infinity.js
  ../internals/math-trunc.js
../internals/to-length.js
  ../internals/to-integer-or-infinity.js
../internals/to-object.js
  ../internals/require-object-coercible.js
../internals/to-primitive.js
  ../internals/function-call.js
  ../internals/get-method.js
  ../internals/is-object.js
  ../internals/is-symbol.js
  ../internals/ordinary-to-primitive.js
  ../internals/well-known-symbol.js
../internals/to-property-key.js
  ../internals/is-symbol.js
  ../internals/to-primitive.js
../internals/try-to-string.js
../internals/uid.js
  ../internals/function-uncurry-this.js
../internals/use-symbol-as-uid.js
  ../internals/symbol-constructor-detection.js
../internals/v8-prototype-define-bug.js
  ../internals/descriptors.js
  ../internals/fails.js
../internals/weak-map-basic-detection.js
  ../internals/global.js
  ../internals/is-callable.js
../internals/well-known-symbol.js
  ../internals/global.js
  ../internals/has-own-property.js
  ../internals/shared.js
  ../internals/symbol-constructor-detection.js
  ../internals/uid.js
  ../internals/use-symbol-as-uid.js

Hope that's helpful.

colinrotherham commented 1 month ago

Lots of Babel + core-js polyfills are included not due to support but for compatibility

But the biggest problem might be op_mob 73 (Opera Mobile 73) included in the targets? See in commit https://github.com/alphagov/govuk-frontend/pull/4557/commits/6b1bf239721983aa7b4156fe8ee42dbc89453e7b how it brought in many more unnecessary changes 😣

Maybe my previous notes in https://github.com/alphagov/govuk-frontend/pull/4557 would be helpful?

Some of the findings explained why polyfills for already-supported features were added:

Findings

Lots more polyfills are added by core-js when compared to ES shims in https://github.com/alphagov/govuk-frontend/pull/4301

We also have lots more unwanted bug fixes:

core-js adds Array.prototype.from polyfill because the type being iterated is unknown

core-js adds Array.prototype.includes polyfill to avoid bugs

core-js adds Error.prototype.cause polyfill to avoid bugs

core-js adds URL constructor polyfill to avoid bugs

Including an overwhelming amount of polyfills to patch various browser APIs in already-supported features:
'es.array.includes',
'es.array.iterator',
'es.regexp.exec',
'es.regexp.to-string',
'es.string.includes',
'es.string.match',
'es.string.replace',
'es.string.trim',
For example, Safari 12.1 updated ''.trim() to include revised empty space characters: https://github.com/paulmillr/es6-shim/pull/354#issuecomment-115565060

Similarly, versions below Firefox 102 handled [].includes() with sparse arrays incorrectly: https://bugzilla.mozilla.org/show_bug.cgi?id=1767541

dracos commented 1 month ago

Even for compatibility, I don't think any of the extra things being included matter to this library, none of them is fixing any actual compatibility issue with the use of this library as far as I can see, and going further, I personally don't think it should even be up to a library to deal with compatibility with a multitude of apparent core "bugs" - otherwise you end up with every library you use being much larger than it needs to be. Why does it matter if trim() trims \x85 and \x200b in one version and not in another? :) IMO, this is spec-purity for no practical use, weighed up against the increased weight of every single web page using a library. (Which I think agrees with your findings, to be clear :) )

tvararu commented 1 month ago

@dracos yeah I agree, instead it would be better if the library specified browser compatibility in the README and let end-users polyfill what they need

alphagov / accessible-autocomplete

Bundle size query (and 8kB version) #712

Findings