Skip to content

Added entries, foreach, values and keys to NodeListOf #14641

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

Merged
merged 11 commits into from
Mar 15, 2017
Merged

Added entries, foreach, values and keys to NodeListOf #14641

merged 11 commits into from
Mar 15, 2017

Conversation

cedvdb
Copy link
Contributor

@cedvdb cedvdb commented Mar 13, 2017

Added entries, foreach, values and keys to NodeListOf

@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@mhegazy
Copy link
Contributor

mhegazy commented Mar 13, 2017

Thanks for your PR. these files are not manually edited, they are auto-generatd from a script in . You can find more information about contributing lib.d.ts fixes at https://github.com/Microsoft/TSJS-lib-generator.

Changes should be done in src/lib instead.

As for this change specifically, it should be done in https://github.com/Microsoft/TypeScript/blob/master/src/lib/dom.iterable.d.ts

@cedvdb
Copy link
Contributor Author

cedvdb commented Mar 13, 2017

@mhegazy I'm confused, should I add it to NodeListOf or NodeList ? Also should I use IterableIterator instead of ArrayLike ?

@cedvdb cedvdb closed this Mar 13, 2017
@cedvdb cedvdb reopened this Mar 13, 2017
@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link

@cedvdb, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@@ -9,5 +9,9 @@ interface NodeList {
}

interface NodeListOf<TNode extends Node> {
keys(): IterableIterator<number>;
Copy link
Contributor

Choose a reason for hiding this comment

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

i would add it also to NodeList.

@@ -5,9 +5,17 @@ interface DOMTokenList {
}

interface NodeList {
keys(): IterableIterator<number>;
values(): IterableIterator<[number, Node]>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for not catching this erlier, but i think these are reversed:

    /** 
      * Returns an array of key, value pairs for every entry in the list
      */
    entries(): IterableIterator<[number, Node]>;

    /** 
      * Returns an list of keys in the list
      */
    keys(): IterableIterator<number>;

    /** 
      * Returns an list of values in the list
      */
    values(): IterableIterator<Node>;
}

keys(): IterableIterator<number>;
values(): IterableIterator<[number, Node]>;
entries(): IterableIterator<Node>;
forEach(): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

From https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach, this should be:

    /**
      * Performs the specified action for each node in an list.
      * @param callbackfn  A function that accepts up to three arguments. forEach calls the callbackfn function one time for each element in the list.
      * @param thisArg  An object to which the this keyword can refer in the callbackfn function. If thisArg is omitted, undefined is used as the this value.
      */
     forEach(callbackfn: (value: Node, index: number, listObj: NodeList) => void, thisArg?: any): void;

keys(): IterableIterator<number>;
values(): IterableIterator<[number, Node]>;
entries(): IterableIterator<Node>;
forEach(callbackfn: (value: Node, index: number, listObj: NodeList) => void, thisArg?: any): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

would also appreciate it if you added the comments i sugested. this way users get nice help message to read instead of going to MDN.

@cedvdb
Copy link
Contributor Author

cedvdb commented Mar 15, 2017

@mhegazy Sorry but I think that the "request changes" are wrongfully blocking this. The return values for entries and values are swapped. https://developer.mozilla.org/en-US/docs/Web/API/NodeList/entries

@mhegazy mhegazy merged commit 4b3cd6a into microsoft:master Mar 15, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Mar 15, 2017

thanks!

@akrillo89
Copy link

The methods forEach of NodeList is node defined.

https://www.w3.org/TR/dom/#interface-nodelist

Therefore where comes this requirement from ? It works for V8 but not for all browsers.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants