refactor: Improve exports & require statements by brody2consult · Pull Request #233 · xmldom/xmldom

brody2consult · 2021-05-09T21:14:22Z

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 · 2021-05-11T04:29:32Z

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 · 2021-08-21T20:37:38Z

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

Can you please reply to my question?
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

brody2consult · 2021-08-24T18:26:04Z

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 · 2021-08-24T19:56:44Z

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?

…e-exports-and-require-statements

…ser.js" This reverts commit d52da16.

….js" This reverts commit d4e1e0e.

This reverts commit b9d8c4e.

Co-authored-by: Christian Bewernitz <[email protected]>

brody2consult · 2021-08-24T20:24:40Z

+ */
+exports.XMLSerializer = dom.XMLSerializer;
 exports.DOMParser = DOMParser;
 exports.__DOMHandler = DOMHandler;


@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?

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.

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.

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 · 2021-08-24T20:24:42Z

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

brody2consult · 2021-08-24T20:37:16Z

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

karfau

Perfect, let's merge this!

brody2consult · 2021-08-24T20:50:12Z

@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 · 2021-08-24T21:01:20Z

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

brody2consult mentioned this pull request May 9, 2021

refactor: move all require statements to beginning of lib/dom-parser.js #195

Closed

brody2consult marked this pull request as ready for review May 9, 2021 21:24

brody2consult requested a review from karfau May 9, 2021 21:24

brody2consult mentioned this pull request May 9, 2021

feat: Update DOMImplementation according to recent specs #210

Merged

karfau reviewed May 11, 2021

View reviewed changes

karfau mentioned this pull request Aug 21, 2021

Cleaner module exports with index.js #36

Closed

karfau added breaking change Some thing that requires a version bump due to breaking changes question Contains open questions that need to be answered labels Aug 21, 2021

karfau added this to the next release with breaking changes milestone Aug 21, 2021

karfau linked an issue Aug 21, 2021 that may be closed by this pull request

breaking: fix main package entry #53

Closed

karfau mentioned this pull request Aug 21, 2021

Additional public Level 2 DOM interfaces #41

Closed

Christopher J. Brody added 5 commits August 24, 2021 15:23

refactor: export items through lib/index.js

5bc3a88

require from main lib index in some tests

18bc0fa

refactor!: remove extra exports from lib/dom-parser.js

b9d8c4e

refactor: move requires & vars to beginning of lib/dom-parser.js

d4e1e0e

cleanup: remove commented lines around exports in lib/dom-parser.js

d52da16

brody2consult force-pushed the improve-exports-and-require-statements branch from 9c73dc2 to d52da16 Compare August 24, 2021 19:28

Christopher J. Brody added 5 commits August 24, 2021 15:59

Merge branch 'master' of https://github.com/xmldom/xmldom into improv…

18b6acb

…e-exports-and-require-statements

Revert "cleanup: remove commented lines around exports in lib/dom-par…

7b29d41

…ser.js" This reverts commit d52da16.

Revert "refactor: move requires & vars to beginning of lib/dom-parser…

12a73d1

….js" This reverts commit d4e1e0e.

Revert "refactor!: remove extra exports from lib/dom-parser.js"

d214737

This reverts commit b9d8c4e.

refactor: move requires & vars to beginning of lib/dom-parser.js

d7a6413

brody2consult removed question Contains open questions that need to be answered breaking change Some thing that requires a version bump due to breaking changes labels Aug 24, 2021

brody2consult removed this from the next release with breaking changes milestone Aug 24, 2021

brody2consult marked this pull request as draft August 24, 2021 20:14

Update lib/dom-parser.js - use @deprecated (2)

a603d3c

Co-authored-by: Christian Bewernitz <[email protected]>

karfau changed the title ~~refactor: improve exports and require statements~~ refactor: Improve exports and require statements Aug 24, 2021

Update lib/index.js - single require for dom module

228df03

Co-authored-by: Christian Bewernitz <[email protected]>

karfau added this to the next release milestone Aug 24, 2021

Update test/dom/serializer.test.js - merge require

8f1b32c

Co-authored-by: Christian Bewernitz <[email protected]>

karfau removed a link to an issue Aug 24, 2021

breaking: fix main package entry #53

Closed

karfau mentioned this pull request Aug 24, 2021

breaking: fix main package entry #53

Closed

brody2consult commented Aug 24, 2021

View reviewed changes

cleanup exports

8bbcd81

brody2consult self-assigned this Aug 24, 2021

brody2consult marked this pull request as ready for review August 24, 2021 20:27

brody2consult changed the title ~~refactor: Improve exports and require statements~~ refactor: Improve exports & require statements Aug 24, 2021

karfau approved these changes Aug 24, 2021

View reviewed changes

brody2consult merged commit dc429ae into xmldom:master Aug 24, 2021

brody2consult deleted the improve-exports-and-require-statements branch August 24, 2021 20:46

Uh oh!

Conversation

brody2consult commented May 9, 2021 • edited by karfau Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karfau left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karfau commented May 11, 2021

Uh oh!

karfau commented Aug 21, 2021

Uh oh!

brody2consult commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karfau commented Aug 24, 2021

Uh oh!

brody2consult Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

karfau Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

brody2consult Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

karfau Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

karfau commented Aug 24, 2021

Uh oh!

brody2consult commented Aug 24, 2021

Uh oh!

karfau left a comment

Choose a reason for hiding this comment

Uh oh!

brody2consult commented Aug 24, 2021

Uh oh!

karfau commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

brody2consult commented May 9, 2021 •

edited by karfau

Loading

karfau left a comment •

edited

Loading

brody2consult commented Aug 24, 2021 •

edited

Loading