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

feat: expose all DOM level 2 element prototypes #637

Merged
merged 7 commits into from
Mar 26, 2024

Conversation

Ponynjaa
Copy link
Contributor

@Ponynjaa Ponynjaa commented Mar 18, 2024

Closes #40.

Exposes all fundamental and extended interfaces according to the specs while keeping the constructors for Node, Document, Element, Attr, CharacterData, Text, Comment, CDATASection, DocumentType, Notation, Entity, EntityReference, DocumentFragment, ProcessingInstruction private by checking a symbol in the constructor.

@Ponynjaa
Copy link
Contributor Author

Ponynjaa commented Mar 18, 2024

I exposed all fundamental and extended interfaces according to the specs while keeping the constructors for almost every class private (I think only DOMException, DOMImplementation, NamedNodeMap and NodeList currently don't check for the internal symbol).

I didn't add tests for the changes yet as I wanted to ask you how you want to handle this first. Do you want a new file just for testing the constructors or do you want to add a file for each class?

Also you mentioned here that you want to expose the classes under "DOM". However I think it would also be fine to just expose them directly, so you can import them like this:

import { Document, Element } from '@xmldom/xmldom';

rather than having to import them like this:

import { DOM } from '@xmldom/xmldom';
const { Document, Element } = DOM;

I think other libraries also just expose them right away, at least I know it this way.
Also I'm not sure what should be exported under "DOM" if we use this approach. I exposed everything according to the specs under DOM right now, but now there's duplicates for DOMException and DOMImplementation.

Also I forgot to apply the changes to some existing tests… I‘m gonna do that tomorrow 😅

@karfau
Copy link
Member

karfau commented Mar 19, 2024

Great stuff, will review it in detail later, when the tests are passing.

Since we now have an approach for private constructors, there is no need for the DOM namespace.

The public constructors that are left now are not an issue from my perspective.

Regarding tests: I think I would prefer to have a check for each "class" in the related file. Can even be the first test in each file.

Is the symbol is even private for "us", meaning we don't even have access to it in the tests and have to use the DOMImplementation to create them?
I guess that makes the most sense, right?
Update: just checked your implementation: By moving the symbol to lib/dom.js we wouldn't need to export it at all, right? Of course that has some implications on tests, but it also means nobody can do
import {INTERNAL_SYMBOL} from " @xmldom/xmldom/lib/conventions".
What do you think?

A last remark: whatever is in the PR description usually goes into the commit message when squash merging. So we should avoid quwstions and instead describe the reasoning behind the changes. Questions can best be put in the comments. Would be nice if you can change that. Thx

PS: you don't need to mention me, I'm watching the repo for every change.

lib/errors.js Outdated Show resolved Hide resolved
lib/conventions.js Outdated Show resolved Hide resolved
lib/dom.js Outdated Show resolved Hide resolved
lib/errors.js Fixed Show fixed Hide fixed
@Ponynjaa
Copy link
Contributor Author

Hey @karfau,
I implemented your suggestions and wanted to start with implementing the tests. I rewrote the existing tests to use document.createXXX for the creation of the different Nodes. This seems to work fine, those tests pass. However almost all of the tests who expect some kind of Exception fail... I don't see the problem, maybe you can have a look at it?

test/dom/attr.test.js Outdated Show resolved Hide resolved
lib/errors.js Show resolved Hide resolved
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 98.73418% with 1 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (master@8f206f7). Click here to learn what that means.

Files Patch % Lines
lib/dom.js 98.18% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #637   +/-   ##
=========================================
  Coverage          ?   94.27%           
=========================================
  Files             ?        8           
  Lines             ?     2096           
  Branches          ?      537           
=========================================
  Hits              ?     1976           
  Misses            ?      120           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@karfau karfau changed the title Expose all DOM level 2 elements (fundamental + extended interfaces) feat: expose all DOM level 2 element prototypes Mar 20, 2024
@karfau karfau enabled auto-merge (squash) March 26, 2024 06:15
Copy link
Member

@karfau karfau left a comment

Choose a reason for hiding this comment

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

Thx

@karfau karfau disabled auto-merge March 26, 2024 06:22
@karfau karfau enabled auto-merge (squash) March 26, 2024 19:30
@karfau karfau merged commit 3248031 into xmldom:master Mar 26, 2024
37 checks passed
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.

Expose DOM Level 2 Elements
2 participants