-
-
Notifications
You must be signed in to change notification settings - Fork 236
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
Move entire codebase to ESM #351
Conversation
@43081j , a minor suggestion. Could you add this change to your pull-request, please? It allows to use ESM-only version of parse5 transparently with ESM-only infrastructure of unifiedjs ecosystem, namely hast-util-from-parse5. A tiny change that saves time for many developers. |
is there an explanation somewhere as to why unifiedjs needs a default export? what prevents you from importing a named export? |
this is not related to unified. It’s related to bundlers not being spec (or Node) compliant. |
im still not following, why would a bundler struggle with a named export? webpack, rollup, esbuild, etc handle named exports just fine afaik. does one of you have a concrete example of some situation that won't work with a named export? |
I’m guessing the issue is that this project is currently CJS and not all bundlers agree how to use CJS inside ESM. All unified projects are already ESM-only. Node.js lets folks import CJS as if they had a default export of the @Konakona-chan Can you confirm or expand on what your issue is? |
No-no-no, named exports work perfectly fine! I just wanted to point out, that out of all breaking changes that you have mentioned in your opening post, the most unjustified one is this one. Previously it was possible to |
ah so its about backwards compatibility. in that case, to be honest its one of probably many breaking changes in this PR. i think regardless of if that particular export is a default or not, consumers would be rewriting a fair amount of code. that particular change is a preference more than anything, but also to bring some consistency to the codebase rather than a big mixture of defaults and named exports. there's a few good reasons for avoiding default exports (worth a google). im skeptical of if inikulin will ever reappear and take a PR like this on anyway, a fork may be needed eventually which would need consumers to rewrite anyway. |
@43081j , thank you for your answer (yeah, I see the pull-request is kind of stale, unfortunately)! Just a note: after changing only one line this pull-request works great (well, I haven't seen any issue yet) in svelte/vite/unifiedjs ESM-only project. Good luck with reviewers in this pull request or forking. |
that's awesome to hear. i haven't really had chance to try it myself yet in any real world projects to see how it runs/builds/etc. i'll have a look at creating a fork some time soon i think. ill keep it up to date with parse5 (if this repo gets updates still) in case we can ever merge it back in. we can at least start using it then also, it does work in a browser just fine! its only the stream package which doesn't but thats fine since it exists solely to pair with node streams. |
The benchmark script still had some broken imports; see parse5@f8afdbb for a fix. |
good catch, have pushed a fix. still haven't had any time recently to sort out the direction of this. seems likely i will fork at some point but i feel like that should come with a bunch of cleanup too (though maybe as a next phase). would also like to move it to typescript, but there's a crazy amount of dynamic |
@43081j I have created a fork at https://github.com/parse5/parse5-fork the other day, and have invited you to join it. This PR, plus some other small updates, are already on I have created a discussion of what to do with the fork at parse5#2 — let's discuss next steps there. |
Thanks for this @43081j! |
Oh, this is a shame. So jsdom cannot depend on parse5 anymore? |
Hi @domenic, just for context: Could you summarise the stance of jsdom with regards to ESM? I am generally okay with delaying ESM in parse5 if this is a blocker somewhere else. |
jsdom has many CommonJS-only dependencies, so it will be CommonJS until all of its dependencies update and the maintainers have time for a conversion. It'd be much preferable if this package supported CommonJS (e.g. via one of the techniques here) until that time; it seems like the only way that we're going to be able to move the ecosystem is with such hybrid strategies. |
Makes sense. I've opened #379 to track this. |
I can understand wanting to stick with CJS, but I don’t quite get this argument:
You want to wait for all users of JSDOM to use ESM or dual, before JSDOM can switch to ESM? 🤔 I find that a weird argument, because:
What about these alternatives:
|
@domenic is right though, in that until JSDOM's dependencies are also ESM, jsdom isn't technically ESM (it'd depend on node interop). jsdom could deliver a CJS bundle, but its still a pretty big chunk of work for them to convert their codebase to ESM. old unmaintained packages tend to get replaced soon enough, fortunately. but its true thats often a blocker for moving to ESM if there's no replacement. we could temporarily ship a CJS bundle until consumers have had time to move, that isn't difficult (have two tsc configs or use a bundler to convert it back to commonjs, then use some package exports to expose it). if for some reason we decided not to ship CJS, i can't see us introducing any extra features anyway so @domenic could probably pin to 6.x. Anything we add really will be code cleanup, refactoring, performance, etc. |
No. I need to wait for all dependencies of jsdom to use ESM or dual. Because ESM packages cannot depend on non-ESM packages without making package startup async (which is not possible with jsdom's architecture). |
As parse5 is a dependency of jsdom, and you are waiting for dependencies of jsdom to use ESM or dual, then why do you now not want parse5 to become ESM?
Could you elaborate? I have experience with ESM packages in native Node.js that use CJS, dual, and ESM dependencies with sync import statements. What if those also land on the current (CJS) version? (And future security stuff of course) |
@domenic totally missed those issues, my bad. tbh i think we should just create a CJS bundle at build time until everyone's moved over. we don't want to block each other's progression to ESM and its a very simple thing for us to add. |
Co-authored-by: Felix Böhm <[email protected]>
Here i've moved the whole codebase to be written as ES modules.
This would be a huge breaking change. I have dropped all commonjs support.
This also resulted in fixing several bugs and bad tests that went unnoticed with CJS.
The aim here is to do another separate change which also moves the whole repo to typescript.
Again, this is a massive breaking change, completely backwards-incompatible by choice. If you don't want it, that is perfectly fine, i can keep it in a fork. If there's any changes and you want to help get it back into this repo, though, im happy to discuss/change whatever.
FYI all tests pass in its current state.
Note also, this probably will not work in browsers. I'd love a world where node can handle true ESM, but it can't. This relies on node-only interop so our dependencies work (as our dependencies still use commonjs). In order to have a true esm codebase, all dependencies would also need to migrate.
cc @inikulin