refactor: Improve exports & require statements

[brodybits]

by creating a proper main entry file lib/index.js.
NOTE that the following exports have been deprecated in lib/dom-parser.js and will be removed in the next minor release:

• DOMImplementation
• XMLSerializer

[karfau]

My understanding of what this change is good for is very limited. Maybe you can elaborate on the problem this solves in the PR description or add a link to something that explains it?

From what I know, such a file can be used to declare the "public API" from the "internal" exports of other modules.
But if that is the goal, I don't really understand why (all) the tests should go through the public API.
We never discussed what thing should be considered "public" and which not. Which is relevant for me when we need to resolve conflicts with the PRs that add exports to some modules in lib.

[karfau]

BREAKING CHANGE due to exports removed from lib/dom-parser.js

Just thinking out loud:
Since lib/dom-parser.js was the main entry point when importing/requiring xmldom can we assume that nobody would write xmldom/lib/dom-parser instead?
If that is the case, since all the exports that previously existed an the main entry point also exist on the new one, this is not a breaking change? (Or did I miss something?)
I also think when doing breaking changes we should be more explicit by listing the exports that have been removed.

[karfau]

@brodybits
&TLDR; I would love to land this asap, but as long as it's a breaking change, it's not that easy.

1. Can you please reply to my question?
2. could you either rebase this branch and resolve the conflicts or push the branch to the xmldom repo, so it's easier for me to continue working on it?

Thx

[brodybits]

Since lib/dom-parser.js was the main entry point when importing/requiring xmldom can we assume that nobody would write xmldom/lib/dom-parser instead?

probably yes

I also think when doing breaking changes we should be more explicit by listing the exports that have been removed.

Done in the now updated description (it was just XMLSerializer).

Yeah I don't think this would likely break any very many real users, definitely should not be in a patch release though.

2. could you either rebase this branch and resolve the conflicts or [...]

I will resolve merge and/or rebase to resolve the conflicts.

In terms of coding it would probably be easier to start this over. But merge & rebase is better for the PR flow. Coming up.

---

P.S. Multiple exports were removed, one was behind a var declaration.

[karfau]

The exports that you listed have been removed from lib/dom-parser.js which was the main enrty point, but that are still present on the main entry point, right?

I would argue that importing things from @xmldom/xmldom/lib/dom-parser (instead of just @xmldom/xmldom) should not be considered part of the "public API", so I wouldn't consider it a breaking change.

And if we can not agree on that, we could make it non breaking for now by restoring those exports and marking them as @deprecated, so people can become aware and we can land it and release it as a patch version, and only remove them in a future PR.

What do you think?

[brodybits]

@karfau this PR made me start to wonder why we are exporting DOMHandler with __ prefix. I recall there was some reason for this when we made the security update for 0.5.0 but embarrassed I do not recall exactly. Can you help me understand this again?

[karfau]

Sure, it was my primitive way to indicate a "private API" and to be able to use it in tests.

Now that we have a clear separation of public (lib/index.js) and private (all other modules) API, we could drop the __ and just export/require DOMHandler.

[brodybits]

Oh yeah !!

I am thinking that if we drop the __ here, it could be easy for a reader to confuse between what is exported internally (for internal testing) and what is to be exported externally.

I am also thinking it would be ideal if we could find a way to make the pluggable DOMHandler usable from the external API somehow.

There are some (myself included) who think that test mocking is a kind of code smell: https://medium.com/javascript-scene/mocking-is-a-code-smell-944a70c90a6a

I would like to propose that we just leave it for now and revisit someday later.

[karfau]

I'm fine with that.
I think mocking needs to be applied with much care, which I hope I did, to avoid brittle and unmaintainable tests.
But of course I'm open to suggestions on how to test those things in a different way.

[karfau]

I assume you will mark it as ready for review and land it when you are ready :)

[brodybits]

@karfau can you take one last look at the exports before I merge this?

[karfau]

Perfect, let's merge this!

[brodybits]

@karfau I accidentally removed you from co-author credit when I merged this, my apologies. I don't know if there is anything we can do about this in git. Argh!!

[karfau]

@brodybits 😆 no problem, you did the main work and I have more than enough credit(s?) in this project