Issue on dependencies resolution

[JSteunou]

I think there is an issue on the dependencies resolution, enlighten in last Chrome, around Iterator.

See this https://polyfill.io/v2/polyfill.js?features=default,es6,es7 in Chrome 61 for example.

/* Polyfill service v3.21.3
 * For detailed credits and licence information see https://github.com/financial-times/polyfill-service.
 * 
 * UA detected: chrome/61.0.0
 * Features requested: default,es6,es7
 * 
 * - Array.prototype.values, License: MIT (required by "es6")
 * - _Iterator, License: MIT (required by "_ArrayIterator", "Array.prototype.@@iterator", "Array.prototype.values", "es6")
 * - String.prototype.contains, License: CC0 (required by "_ArrayIterator", "Array.prototype.@@iterator", "Array.prototype.values", "es6")
 * - _ArrayIterator, License: MIT (required by "Array.prototype.@@iterator", "Array.prototype.values", "es6") */

...

and this goes on with polyfills for Array.prototype.values, _Iterator, _ArrayIterator, String.prototype.contains

If we look at Array.prototype.values it actually should be polyfilled in Chrome 61, the statement is correct. It depends on Array.prototype.@@iterator and Symbol.iterator which depends on _ArrayIterator

Looking back at the polyfill generated, the header tells us that _Iterator and _ArrayIterator are included because of Array.prototype.values which is a falsy statement.

The entire chain is Array.prototype.values --> Array.prototype.@@iterator --> _ArrayIterator --> _Iterator and this chain should be broke at Array.prototype.@@iterator with Chrome 61 because it supports it.

This make polyfill-service include more useless stuff, and some cannot be gated.

[JakeChampion]

@JSteunou Thanks for the issue. The chain didn't include Array.prototype.@@iterator as it is supported in your version of chrome. The root cause of this issue, as you already know is that some polyfills are not gated and have misconfigured browser support ranges. The _ArrayIterator polyfill should not be served to chrome 61 but is currently configured to serve to all Chrome versions.

Could you list which polyfills cannot be gated please? I will look into ensuring we can gate them all.

[JSteunou]

The _ArrayIterator polyfill should not be served to chrome 61 but is currently configured to serve to all Chrome versions.

Is there a particular reason _ArrayIterator is served to everyone, instead of just being served only when one required polyfill requires it? If it is included only when required, as Chome 61 does not require Array.prototype.@@iterator it would not be included.

What I find odd though, is the header statement, telling us that ArrayIterator is included because of Array.prototype.values and not because Chrome 61 is in Chrome *

[JSteunou]

Could you list which polyfills cannot be gated please? I will look into ensuring we can gate them all.

A simple walking tree script checking presence of config.json but absence of detect.js should do the trick. I dont know when I would have the time to do it, but I could look at it.

[JSteunou]

@JakeChampion I still think there is a dependency issue independent to gated.

Let me explain better. I'm Chrome 61, requesting polyfill for es6,es7. The service analyses the UA and the features requested

what's in es6,es7 not supported by Chrome 61?

• Array.prototype.values
• String.prototype.contains

Ok, what those features directly depends on...

• Symbol.iterator
• Array.prototype.@@iterator
• String.prototype.includes

...which of those are not supported by Chrome 61?

none

Yes, nothing because Symbol.Iterator is supported since Chrome 50, Array.prototype.@@iterator is supported since Chrome 49, and String.prototype.includes since Chrome 41.

So why is the service providing _ArrayIterator ? Because there is a flaw in the dependencies computation.

I think it is included only because of the browsers property including all browsers, with a property aliases to undefined whereas it should only be included when needed by another polyfill

[JSteunou]

Did a quick digging, when filtering by UA _ArrayIterator is flagged as aliasOf es6, which it should not

  _ArrayIterator: 
   { flags: Set { 'gated' },
     aliasOf: Set { 'Array.prototype.@@iterator', 'Array.prototype.values', 'es6' } },

To be continued...

[JSteunou]

Ok find why and where. Let's keep our example with Chrome 61 and features es6,es7

All start here https://github.com/Financial-Times/polyfill-service/blob/master/lib/index.js#L111
This code will try to find which polyfill to include by

1. resolving aliases (in our case es6,es7)
2. then filter by UA (Chrome 61)
3. then recursively adding all dependencies
4. then filter again by UA on all dependencies
5. then filter by exclude, which in our case does not matter

Step by step analyse

1. resolving aliases

It will just return a feature list like this one, generated from the aliases.json

[ 'Array.from',
  'Array.of',
  'Array.prototype.@@iterator',
  'Array.prototype.entries',
  'Array.prototype.fill',
  'Array.prototype.find',
  'Array.prototype.findIndex',
  'Array.prototype.keys',
  'Array.prototype.values',
  'Function.name',
  'Map',
  'Math.acosh',
  'Math.asinh',
  'Math.atanh',
  'Math.cbrt',
  'Math.clz32',
  'Math.cosh',
  'Math.expm1',
  'Math.hypot',
  'Math.imul',
  'Math.log10',
  'Math.log1p',
  'Math.log2',
  'Math.sign',
  'Math.sinh',
  'Math.tanh',
  'Math.trunc',
  'Number.MAX_SAFE_INTEGER',
  'Number.MIN_SAFE_INTEGER',
  'Number.isFinite',
  'Number.isInteger',
  'Number.isNaN',
  'Number.parseFloat',
  'Number.parseInt',
  'Object.assign',
  'Object.is',
  'Object.setPrototypeOf',
  'Promise',
  'Set',
  'String.prototype.@@iterator',
  'String.prototype.endsWith',
  'String.prototype.includes',
  'String.prototype.repeat',
  'String.prototype.startsWith',
  'Symbol.hasInstance',
  'Symbol.isConcatSpreadable',
  'Symbol.iterator',
  'Symbol.match',
  'Symbol.replace',
  'Symbol.search',
  'Symbol.species',
  'Symbol.split',
  'Symbol.toPrimitive',
  'Symbol.toStringTag',
  'Symbol.unscopables',
  'Symbol',
  'WeakMap',
  'WeakSet',
  'Array.prototype.includes',
  'String.prototype.padEnd',
  'String.prototype.padStart' ]

no issue there

2. Filtering by UA

We are using Chrome 61 it keeps Array.prototype.values which is good.

3. Getting all dependencies

Our case is easier to debug because we have just one method to polyfill. It finds this list of dependencies, each one having metadata

Array.prototype.values
Symbol.iterator
Array.prototype.@@iterator
Object.defineProperty
Symbol
_ArrayIterator
Array.prototype.forEach
Array.prototype.filter
Array.prototype.map
Object.create
Object.getOwnPropertyNames
Object.getOwnPropertyDescriptor
Object.freeze
Object.keys
_Iterator
Object.setPrototypeOf
String.prototype.contains
Object.defineProperties
Function.prototype.bind
Object.assign
Symbol.toStringTag
Object.getPrototypeOf
String.prototype.includes

Notice that _ArrayIterator makes it 1st apparition, because it is a dependency of Array.prototype.@@iterator which is a dependency of Array.prototype.values, which is fine.

4. Filtering by UA

And this is where the issue happens. All features supported by Chrome 61 are filter out, but utilities like _ArrayIterator are kept because they are flagged as not supported by everyone, whereas Array.prototype.@@iterator is correctly filtered out.

_ArrayIterator is not needed anymore but kept, along with its dependencies. There is our root issue.

How to solve this?

I propose to add another step of resolving, instead of

1. resolving aliases (in our case es6,es7)
2. then filter by UA (Chrome 61)
3. then recursively adding all dependencies
4. then filter again by UA on all dependencies
5. then filter by exclude, which in our case does not matter

it should be

1. resolving aliases (in our case es6,es7)
2. then filter by UA (Chrome 61)
3. then recursively adding all dependencies except utilities
4. then filter again by UA on all dependencies
5. then recursively adding all utilities dependencies
6. then filter by exclude, which in our case does not matter

But you might have to

• loop between 5 and 3 if those utilities depend on other polyfill themself, until reaching out the full tree of dependencies.
• add a new metadata entry / key for utilities

[JSteunou]

A less consuming way to solve this would be to filter by UA while recursively adding all dependencies so unneeded dependencies aren't not added and so never utilities when not needed.

1. resolving aliases (in our case es6,es7)
2. then filter by UA (Chrome 61)
3. then recursively find, filter by UA & adding all dependencies
4. then filter by exclude, which in our case does not matter

[JSteunou]

@JakeChampion any bandwith to take a look?

[radeno]

Any progress of this? I think it is really complicated issue, because some dependencies are not striped properly.

For example this:
https://cdn.polyfill.io/v2/polyfill.js?features=es6,es7,es8&excludes=Array.prototype.values&flags=gated

It returns for Chrome:

 * UA detected: chrome/62.0.0
 * Features requested: es6,es7,es8
 * 
 * - _Iterator, License: MIT (required by "_ArrayIterator", "Array.prototype.@@iterator", "Array.prototype.values", "es6")
 * - String.prototype.contains, License: CC0 (required by "_ArrayIterator", "Array.prototype.@@iterator", "Array.prototype.values", "es6")
 * - _ArrayIterator, License: MIT (required by "Array.prototype.@@iterator", "Array.prototype.values", "es6") */

for Firefox:

 * UA detected: firefox/57.0.0
 * Features requested: es6,es7,es8
 * 
 * - _Iterator, License: MIT (required by "_ArrayIterator", "Array.prototype.@@iterator", "Array.prototype.values", "es6")
 * - _ArrayIterator, License: MIT (required by "Array.prototype.@@iterator", "Array.prototype.values", "es6") */

What are issues?

1. String.prototype.contains doesn't exists in ECMAScript specification, it is proprietary implementation.
2. Iterator is always included for every browser version.
3. Gated function doesn't work. I'm excluding Array.prototype.values, but still getting its dependencies.

Possible solutions? (In global, not just my short issue)

1. Remove all deprecated and unofficial implementations from official language features

• it could help to see what is really necessary to include as dependency
• proprietary features should not be included by default

2. Check real supported version

• add most as we can information about supported features
• eg: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/%40%40iterator

3. Better shrinking dependencies as @JSteunou said. Why is _ArrayIterator included when i explicitly exclude Array.prototype.values?

What next?
Pull requests?

[radeno]

Array.prototype.values is now support by most browsers. Need just change version checking opened by this pull request: polyfillpolyfill/polyfill-service#1736

[JakeChampion]

Fixed via #558 and confirmed fixed via #622