Skip to content

fix!: Set nodeName property to ProcessingInstruction#509

Merged
karfau merged 2 commits into
xmldom:masterfrom
cjbarth:processing-instruction-nodename
Jul 12, 2023
Merged

fix!: Set nodeName property to ProcessingInstruction#509
karfau merged 2 commits into
xmldom:masterfrom
cjbarth:processing-instruction-nodename

Conversation

@cjbarth

@cjbarth cjbarth commented Jul 12, 2023

Copy link
Copy Markdown
Contributor

Closes #505

BREAKING CHANGE: (only of 0.9.x) the tagName property is no longer set since a ProcessingInstruction is not an Element

@karfau

karfau commented Jul 12, 2023

Copy link
Copy Markdown
Member

Hey @cjbarth this is great news and an excelent additional set of tests.

I have just one remark:

Please keep the tagName value, since it represents the value from the Element interface:

Thus, even though there is a generic Node.nodeName attribute on the Node interface, there is still a Element.tagName attribute on the Element interface; these two attributes must contain the same value, but it is worthwhile to support both, given the different constituencies the DOM API must satisfy.

https://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-1CED5498

and ideally the tests should also make sure both values are aligned.

(I know it's not ideas to keep a reference to the value twice since they can get out of sync but are currently not able to have setters and getters for all supported runtimes, so that's a general issue in the code base.)

@karfau

karfau commented Jul 12, 2023

Copy link
Copy Markdown
Member

Oh and please run npm run format before commiting, to let prettier handle those "linting" issues. (I think I should finally add a pre-commit hook for that...)

@karfau karfau added spec:DOM Level 3 https://www.w3.org/TR/DOM-Level-3-Core/ testing labels Jul 12, 2023
@karfau karfau added this to the 0.9.0 milestone Jul 12, 2023
@cjbarth

cjbarth commented Jul 12, 2023

Copy link
Copy Markdown
Contributor Author

represents the value from the Element interface

I completely agree that .tagName is a property on the Element interface. However, ProcessingInstruction doesn't inherit from Element, so it doesn't have .tagName.

@cjbarth

cjbarth commented Jul 12, 2023

Copy link
Copy Markdown
Contributor Author

As a side point, I'm curious why this library isn't at 1.x yet. The API seems stable so I would expect a semver 1.0.0 would have been released.

@cjbarth

cjbarth commented Jul 12, 2023

Copy link
Copy Markdown
Contributor Author

please run npm run format before commiting

Done.

@karfau

karfau commented Jul 12, 2023

Copy link
Copy Markdown
Member

Thank you for insisting/clarification.

@karfau

karfau commented Jul 12, 2023

Copy link
Copy Markdown
Member

You can have a look at the milestones that we currently have put all issues in that we are aware if.
So far we have had a decent amount if breaking changes in every feature/minor release and 0.9.0 will also have that, so I think it correctly reflects the current "matureness" of the implementation compared to what the native browser API offers.
There are definitely some differences in behavior compared to the browser native API, which this is claiming to ponyfill, so for a 1.0 I would at least expect to be able to have a web platform test running to make those differences as explicit as possible and have worked towards reducing them to a level that is understandable/managable

@karfau karfau merged commit 42ab526 into xmldom:master Jul 12, 2023
@cjbarth cjbarth deleted the processing-instruction-nodename branch July 12, 2023 17:54
karfau pushed a commit that referenced this pull request Jul 13, 2023
karfau pushed a commit that referenced this pull request Jul 13, 2023
@karfau

karfau commented Jul 13, 2023

Copy link
Copy Markdown
Member

released as 0.7.12(@lts), 0.8.9 (@latest) and 0.9.0-beta.9 (@next) version tag

(0.7.12 and 0.8.9 still set the tagName to make it a non breaking change)

@karfau karfau added the breaking change Some thing that requires a version bump due to breaking changes label Jul 13, 2023
@karfau karfau changed the title Add nodeName property with tests to ProcessingInstruction fix!: Set nodeName property to ProcessingInstruction Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Some thing that requires a version bump due to breaking changes spec:DOM Level 3 https://www.w3.org/TR/DOM-Level-3-Core/ testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ProcessingInstruction missing nodeName property

2 participants