fix: attribute persistence bug (#2) - #3
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes an attribute persistence bug (#2) along with several configuration updates and improvements. The main fix addresses how attributes are managed in the DOMTokenList and NamedNodeMap classes, ensuring proper deduplication and persistence of token values.
Key Changes:
- Fixed attribute persistence bug in
DOMTokenList.replace()by adding deduplication and explicit attribute updates - Refactored
NamedNodeMapmethods to replaceindexOfcalls with manual iteration, and added case-insensitive attribute name comparison - Added CI/CD workflow for automated testing, building, and publishing to JSR/NPM registries
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/collections.ts |
Fixed attribute persistence in DOMTokenList.replace() with deduplication; refactored NamedNodeMap methods (setNamedItem, setNamedItemNS, removeNamedItem, removeNamedItemNS) to use manual loops instead of indexOf, and made attribute name lookups case-insensitive |
deno.json |
Updated build command to change npm package destination; simplified task definitions by converting nested dependencies to command strings |
README.md |
Updated introduction text for clarity; fixed usage example to correctly access h1 element via doc.body.firstElementChild instead of incorrect path |
.github/workflows/ci.yml |
Added comprehensive CI/CD workflow with quality control checks (fmt, lint, test) and automated publishing to JSR, NPM, and GitHub Package Registry |
.gitattributes |
Added git attributes configuration for line endings and linguist vendoring rules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let index = 0; | ||
| for (; index < this.length; index++) { | ||
| if (this.item(index) === existing) break; | ||
| } |
There was a problem hiding this comment.
The manual loop to find the index is less efficient than using the existing indexOfInList helper function. Consider replacing this with: const index = indexOfInList(getListStorage(this), existing); for better performance and code consistency.
| let index = 0; | ||
| for (; index < this.length; index++) { | ||
| if (this.item(index) === existing) break; | ||
| } |
There was a problem hiding this comment.
The manual loop to find the index is less efficient than using the existing indexOfInList helper function. Consider replacing this with: const index = indexOfInList(getListStorage(this), existing); for better performance and code consistency.
| let index = 0; | ||
| for (; index < this.length; index++) { | ||
| if (this.item(index) === existing) break; | ||
| } |
There was a problem hiding this comment.
The manual loop to find the index is less efficient than using the existing indexOfInList helper function. Consider replacing this with: const index = indexOfInList(getListStorage(this), existing); for better performance and code consistency.
| let index = 0; | ||
| for (; index < this.length; index++) { | ||
| if (this.item(index) === existing) break; | ||
| } |
There was a problem hiding this comment.
The manual loop to find the index is less efficient than using the existing indexOfInList helper function. Consider replacing this with: const index = indexOfInList(getListStorage(this), existing); for better performance and code consistency.
| tokens[index] = newToken; | ||
| this.#updateAttribute(); | ||
| this.#tokens = tokens.filter((t, i, a) => indexOf(a, t) === i); | ||
| this.#updateAttribute(this.#tokens.join(" ")); |
There was a problem hiding this comment.
Inconsistent style: The explicit value parameter is redundant since the #updateAttribute method will default to this.value (which calls this.#tokens.join(" ")) when no parameter is provided. The add method on line 1027 and remove method on line 1034 use this.#updateAttribute() without arguments for consistency. Consider removing the parameter to match the pattern used elsewhere.
| this.#updateAttribute(this.#tokens.join(" ")); | |
| this.#updateAttribute(); |
| namedItem(name: string): Attr | null { | ||
| for (const attr of this) { | ||
| if (attr.name === name) return attr; | ||
| if (attr.name.toLowerCase() === name.toLowerCase()) return attr; |
There was a problem hiding this comment.
The case-insensitive comparison may not be correct for all document types. According to the DOM specification, attribute name comparisons should be case-insensitive only for HTML elements in the HTML namespace. For XML documents and other namespaces, attribute names should be compared case-sensitively. Consider checking the document type or element namespace before applying case-insensitive comparison.
| if (attr.name.toLowerCase() === name.toLowerCase()) return attr; | |
| const owner = attr.ownerElement; | |
| if ( | |
| owner && owner.namespaceURI === "http://www.w3.org/1999/xhtml" | |
| ? StringPrototypeToLowerCase(attr.name) === StringPrototypeToLowerCase(name) | |
| : attr.name === name | |
| ) { | |
| return attr; | |
| } |
Fixes #2.