Skip to content

Commit

Permalink
remove forEach from majority of DOM collection prototypes
Browse files Browse the repository at this point in the history 
These appear to be added to address #329. However, only NodeList and
DOMTokenList are documented and tested to have forEach on their
prototypes.
  • Loading branch information
@moorejs
moorejs committed Sep 21, 2021
1 parent ed21ba3 commit c01268a
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion packages/core-js/modules/web.dom-collections.for-each.js
View file
Expand Up @@ -14,7 +14,9 @@ var handlePrototype = function (CollectionPrototype) {
};

for (var COLLECTION_NAME in DOMIterables) {
handlePrototype(global[COLLECTION_NAME] && global[COLLECTION_NAME].prototype);
if (DOMIterables[COLLECTION_NAME]) {
handlePrototype(global[COLLECTION_NAME] && global[COLLECTION_NAME].prototype);
}
}

handlePrototype(DOMTokenListPrototype);

6 comments on commit c01268a

@yulodl
Copy link

@yulodl yulodl commented on c01268a Oct 19, 2021

Choose a reason for hiding this comment

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

This is a break change for accidently using forEach on FileList.

@zloirock
Copy link
Owner

@zloirock zloirock commented on c01268a Oct 19, 2021 •
edited

Choose a reason for hiding this comment

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

I agree, I was sure that some mistakenly used it. But it's not a standard, it was not documented and it was added because of a bug - so it's a bugfix and it was required to add it to prevent more cases of this.

@antoniandre
Copy link

Choose a reason for hiding this comment

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

Hi @zloirock,

if (DOMIterables[COLLECTION_NAME]) {

And based on this list in internals/dom-iterables.js:

// iterable DOM collections
// flag - `iterable` interface - 'entries', 'keys', 'values', 'forEach' methods
module.exports = {
  CSSRuleList: 0,
  CSSStyleDeclaration: 0,
  CSSValueList: 0,
  ClientRectList: 0,
  DOMRectList: 0,
  DOMStringList: 0,
  DOMTokenList: 1,
  DataTransferItemList: 0,
  FileList: 0,
  HTMLAllCollection: 0,
  HTMLCollection: 0,
  HTMLFormElement: 0,
  HTMLSelectElement: 0,
  MediaList: 0,
  MimeTypeArray: 0,
  NamedNodeMap: 0,
  NodeList: 1,
  ...

This means that we add the forEach prototype on all the DOM iterables that already support forEach like the NodeList (https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach), but we don't add support for the ones that don't support it.

Correct me if I'm wrong but shouldn't we do the opposite? if (!DOMIterables[COLLECTION_NAME]) {

After updating to the latest core-js, printing in the console HTMLCollection.prototype does not show the forEach function (and it did in the previous version) whereas NodeList.prototype does.

We can still convert it to an array to benefit from the built-in forEach but if this change is intended, it is a breaking change to not be able to do document.getElementsByClassName('my-class').forEach anymore, like we could before this release. And in this case it should be a major release.

Let me know! :)

@zloirock
Copy link
Owner

Choose a reason for hiding this comment

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

@antoniandre it should be added only to NodeList and DOMTokenList, other DOM collections do not contain it by the spec and adding something non-standard is not something good.

@antoniandre
Copy link

Choose a reason for hiding this comment

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

I understand.
Then it should be documented as a lot of people may get the same problem as me where it was supported before and breaking after update. ;)

@zloirock
Copy link
Owner

@zloirock zloirock commented on c01268a Nov 24, 2021 •
edited

Choose a reason for hiding this comment

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

It's documented that it's added only to NodeList and DOMTokenList, the docs never said something different, this bugfix is documented in the changelog.

Please sign in to comment.