Skip to content
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

fix(purify): fix _createIterator #850

Merged
merged 5 commits into from Aug 11, 2023
Merged

Conversation

ssi02014
Copy link
Contributor

@ssi02014 ssi02014 commented Aug 10, 2023

Summary

Hello 👋, @cure53
I was looking at the _createIterator function and realized it had an unnecessary argument (false), so I did a fix-and-refactor.

https://developer.mozilla.org/en-US/docs/Web/API/Document/createNodeIterator
https://dom.spec.whatwg.org/#dom-document-createnodeiterator

Even if you refer to the documentation above, the createNodeIterator function only requires 3 arguments: root, whatToShow, and filter.

스크린샷 2023-08-11 오전 3 52 40
https://github.com/microsoft/TypeScript/blob/main/src/lib/dom.generated.d.ts#L7160

Tasks

  • fix _createIterator

Comment on lines +903 to +908
* Creates a NodeIterator object that you can use to traverse filtered lists of nodes or elements in a document.
*
* @param {Document} root document/fragment to create iterator for
* @return {Iterator} iterator instance
* @param {Node} root The root element or node to start traversing on.
* @return {NodeIterator} The created NodeIterator
*/
const _createIterator = function (root) {
const _createNodeIterator = function (root) {
Copy link
Contributor Author

@ssi02014 ssi02014 Aug 10, 2023

Choose a reason for hiding this comment

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

https://github.com/microsoft/TypeScript/blob/main/src/lib/dom.generated.d.ts#L7160
I've modified the jsdoc with reference to that documentation.

Also, the naming seems clearer for _createNodeIterator than _createIterator. (Returns a NodeIterator.)
What do you think of this fix? 🙏

@ssi02014 ssi02014 changed the title fix(purify): fix createIterator fix(purify): fix _createIterator Aug 10, 2023
Comment on lines -915 to +914
false
null
Copy link
Contributor Author

@ssi02014 ssi02014 Aug 10, 2023

Choose a reason for hiding this comment

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

I removed that argument. ("false")

*/
const _basicCustomElementTest = function (tagName) {
const _isBasicCustomElement = function (tagName) {
Copy link
Contributor Author

@ssi02014 ssi02014 Aug 10, 2023

Choose a reason for hiding this comment

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

The function name in the jsdoc was different from the actual function name, so I tried to modify it, and finally changed it to _isBasicCustomElement with the is prefix because it returns a boolean.

jsdoc: _basicCustomElementCheck
actual: _basicCustomElementTest

@cure53 cure53 merged commit 1154de2 into cure53:main Aug 11, 2023
6 checks passed
@cure53
Copy link
Owner

cure53 commented Aug 11, 2023

This looks good me thinks, but let me do some manual testing - high risk change 😅

@ssi02014 ssi02014 deleted the fix/createIterator branch August 11, 2023 13:32
@ssi02014
Copy link
Contributor Author

@cure53
Based on the specification, I don't think the last argument, "false", will make any difference in behavior, but it's worth checking.

Thanks!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants