fix: convert nodelist to array before passing to native Array methods #1548
Conversation
| collection: NodeListOf<T> | HTMLCollectionOf<K> | ||
| ): Array<T | K> { | ||
| const size = collection.length; | ||
| const cloned: T[] | K[] = new ArrayConstructor(size); |
There was a problem hiding this comment.
This method performs better than instantiating an empty array and calling ArrayPush.
https://jsperf.com/new-array-vs-push-lwc
| * This is to aviod conflict with some legacy third party libraries like prototype.js | ||
| * that patch Array.prototype and are incapable of handling NodeList and HTMLCollection. | ||
| */ | ||
| export function arrayFromCollection<T extends Node, K extends Element>( |
There was a problem hiding this comment.
can we have two methods, one for htmlcollection and one for nodelist? even if the underlying implementation is the same... I feel that this PR is messing with the types.
| @@ -83,7 +84,7 @@ export function getAllMatches(owner: Element, nodeList: NodeList | Node[]): Arra | |||
| return filteredAndPatched; | |||
| } | |||
|
|
|||
| export function getFirstMatch(owner: Element, nodeList: NodeList): Element | null { | |||
| export function getFirstMatch(owner: Element, nodeList: NodeList | Element[]): Element | null { | |||
There was a problem hiding this comment.
this type extension seems wrong.
There was a problem hiding this comment.
This method is called in two places. One with a NodeList and the other with an Element[]. That is the reason for this change.
BTW, the same pattern is in L73
export function getAllMatches(owner: Element, nodeList: NodeList | Node[]):
| @@ -0,0 +1,24 @@ | |||
| /* | |||
There was a problem hiding this comment.
We should move this utility function to ./utils.ts
| * Issue #1545 - Writing a custom util to convert NodeList and HTMLCollection instances to array. | ||
| * This is to aviod conflict with some legacy third party libraries like prototype.js | ||
| * that patch Array.prototype and are incapable of handling NodeList and HTMLCollection. |
There was a problem hiding this comment.
This utility should be used to convert NodeList and HTMLCollection into an array before we perform array operations on them. See issue #1545 for more details.
packages/@lwc/synthetic-shadow/src/polyfills/document-shadow/polyfill.ts
Show resolved
Hide resolved
packages/@lwc/synthetic-shadow/src/polyfills/document-shadow/polyfill.ts
Show resolved
Hide resolved
packages/@lwc/synthetic-shadow/src/polyfills/document-shadow/polyfill.ts
Show resolved
Hide resolved
packages/@lwc/synthetic-shadow/src/polyfills/document-shadow/polyfill.ts
Show resolved
Hide resolved
packages/@lwc/synthetic-shadow/src/polyfills/document-shadow/polyfill.ts
Show resolved
Hide resolved
ravijayaramappa
force-pushed
the
ravi/master/collection-to-array/W-6684382
branch
from
5682736 to
2661bcf
Compare
Details
Alternative to: #1546
Does this PR introduce breaking changes?
No, it does not introduce breaking changes.If yes, please describe the impact and migration path for existing applications.
The PR fulfills these requirements: