-
Notifications
You must be signed in to change notification settings - Fork 83
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
refactor: Improve exports & require statements #233
refactor: Improve exports & require statements #233
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Just thinking out loud: |
@brodybits
Thx |
probably yes
Done in the now updated description Yeah I don't think this would likely break
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. |
9c73dc2
to
d52da16
Compare
The exports that you listed have been removed from I would argue that importing things from And if we can not agree on that, we could make it non breaking for now by restoring those exports and marking them as What do you think? |
Co-authored-by: Christian Bewernitz <coder@karfau.de>
Co-authored-by: Christian Bewernitz <coder@karfau.de>
Co-authored-by: Christian Bewernitz <coder@karfau.de>
lib/dom-parser.js
Outdated
/** | ||
* @deprecated Import/require from main entry point instead | ||
*/ | ||
exports.XMLSerializer = dom.XMLSerializer; | ||
exports.DOMParser = DOMParser; | ||
exports.__DOMHandler = DOMHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I assume you will mark it as ready for review and land it when you are ready :) |
@karfau can you take one last look at the exports before I merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, let's merge this!
@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!! |
@brodybits 😆 no problem, you did the main work and I have more than enough credit(s?) in this project |
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