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

Transition to ES6? #76

Open
ajmas opened this issue Jun 30, 2020 · 14 comments
Open

Transition to ES6? #76

ajmas opened this issue Jun 30, 2020 · 14 comments

Comments

@ajmas
Copy link
Contributor

ajmas commented Jun 30, 2020

I am curious whether there is any interest to transition the library to use ES6 coding or even TS?

I did look to see if there was a previous ticket on this, but did not see any. Also, given the stability of library I can appreciate that it may not be desirable, though figured I'd file the ticket just so it is asked.

@brodybits
Copy link
Member

brodybits commented Jun 30, 2020

updated:

The motivation for sticking with ES5 is discussed in PR #107. That said, I am thinking it would be nice to look for a better way to support users who need ES5. This would definitely be a breaking change.


original response:

Definitely an interesting idea.

I think we could definitely use some modernization, using "Standard JS" or semistandard, for example (see #29).

In terms of using ES6 import and export statements, which may overlap with #75, it would be a breaking change and maybe require setting module type in package.json, looks not well standardized, or add a pre-publish build step, which I would like to avoid.

I would also downvote switching to TS, since it would both add another build step and add a possible barrier of entry. It is possible to use TypeScript to do type checking with JSDoc type comments, which I would favor looking into.

@ajmas
Copy link
Contributor Author

ajmas commented Jun 30, 2020

Keeping it simple is the best way to go, so no TS makes sense.

Given the minimum target NodeJS version is 10, I imagine the changes could be done in an evolutionary way, before dealing with anything truly breaking. There is already a lot that can be done, assuming that as minimal environment?

Simple things like moving from var to const/let could be easy wins. Since there are tests, this will also help identify anything that breaks functionality?

@karfau
Copy link
Member

karfau commented Jul 1, 2020

Thx for raising the topic.
Sadly I would say that the current tests are very far from being that reliable.
Just from the code that I took a closer look at so far I'm quite sure there is some that might actually rely on the different behavior of var...

I think adding an a TS check on the js code based on JSDoc comments might already reveal many "issues" (also need to be treated with care, any change could be breaking), and it's something I have some experience with.
But I'm not sure this topic actually qualifies for this issue.

@snowteamer
Copy link

Sure keeping it simple is important. However, I would argue that it should be simpler to work with ES6 than with legacy JavaScript.

It is possible to use TypeScript to do type checking with JSDoc type comments

That is true, but unfortunately the TS type syntax support in JsDoc is somewhat limited.

@michaelhkay
Copy link

We use xmldom (at any rate, the dom.js module) in Saxon-JS. We probably forked our copy 3 or 4 years ago, but kept changes to a minimum. We've made a few very small changes to fix minor bugs and to make it compile more nicely under Closure. We've been moving the rest of our code to ES6 and I'm very tempted to do the same with dom.js; it's not clear that we'd lose much by forking. Although there's a fair trickle of activity, there don't seem to be any "must have" changes.

@karfau
Copy link
Member

karfau commented Oct 1, 2020

Even though it's a bit off topic in this issue:
@michaelhkay I think we would be very happy to integrate your bug fixes into this code base. Are you able to file one or more PRs?
If not can you point us to the fork you are talking about, so we can see if we are able to cherry pick?

As to the "moving to ES6", I still think we will need to talk about many things before we can even get started. Since I'm not sure what environments we would break by no longer supporting older browsers.
And if we want to go there and support them we will need some kind of transpilation.
But I'm not so confident that our current test suite would really prevent us from breaking things...

I don't have any issues with typescript and I think we will massively benefit from it even if we just run it over the current code base as part of development/CI. I also think t would also really speed up development, but I think it's a long path to go from where we are right now.
(More contributors of course always help with that.)

@brodybits
Copy link
Member

@michaelhkay I would also be happy with contribution of the bug fixes. Maintaining a fork should ideally be a last resort.

At this point we would be pretty reluctant to switch to ES6 since this would break in some ES5 environments, as discussed in PR #107.

For type checking, I would favor using JSDoc comments rather than switching to TypeScript. I think this would be similar to using Closure. I have contributed some changes to do this in Prettier, and there is a nice writeup here: https://fettblog.eu/typescript-jsdoc-superpowers/

@michaelhkay
Copy link

michaelhkay commented Oct 2, 2020 via email

@brodybits brodybits pinned this issue Jan 21, 2021
@brodybits
Copy link
Member

From recent discussions including recently merged PR #181 I am starting to wonder if we should consider transitioning to ES6 and looking for another way to support ES5 environments that were discussed in PR #107. I think supporting ES6 would be easier and more future-proof.

@shunkica
Copy link
Collaborator

shunkica commented Jul 4, 2023

@karfau What is the status of this?

@michaelhkay
Copy link

I've done a quick comparison of our version of dom.js against the current master. In fact it seems we did very little other than add some diagnostic code. The places we did a few bug fixes were in sax.js, which we use for parsing. The master code has evolved a great deal since we took our snapshot, but we haven't got a strong reason to merge the updates. Longer term we're more likely to cut our own tree model separate from DOM; our experience on the Java side is that you can get significant benefits by using an immutable tree that's more closely integrated with the XPath engine.

@karfau
Copy link
Member

karfau commented Jul 5, 2023

Not any progress other then discussions.
All discussions regarding different supported or unsupported runtimes can be found in #214
And I started a poll regarding node versions to support in #495

One main reason for this not happening is the lack of resources.
I will not even start working on that as long as I'm the only active "core" maintainer. Mainly because of the expected time investment, but also because during the transition any critical bugs/ security issues have to be ported over, which is not just a cherry pick anymore.

If there would ever be enough resources for such a rewrite I would most likely vote for moving to typescript and add a build step to support all the environments people have asked for.

@Ponynjaa
Copy link
Contributor

@karfau I would be down to invest some time into rewriting xmldom in TS if that would help you, as I also see a huge benefit in using TS over plain JS (e.g. #285, #381).

@karfau
Copy link
Member

karfau commented Mar 17, 2024

Just to mention it here, since I think it's quite relevant:
I recently discovered https://github.com/bwrrp/slimdom.js which seems to be a straight forward, well tested alternative which is already written in typescript. The only reason you might not be able to use it is because it completely lacks any "lax" HTML parsing.
Which I can completely understand, since it is a can of worms... and that problem has been solved in other libs like https://github.com/inikulin/parse5

Also other runtimes like Deno and Bun are becoming more popular/relevant, and some of them already bring their own spec compliant implementation(s).
So I'm not sure it's worth the effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants