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

Add happy-dom and benchmark #457

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zirkelc
Copy link

@zirkelc zirkelc commented Mar 23, 2024

Still work in progress.

EDIT:

  • added jsdom for comaprison

This PR adds happy-dom as a potential replacement for domino due to the issues described in #378
A new benchmark test was added in test/benchmark.js. The process.env.PARSER variable toggles between happy-dom and domino.

Current results:

┌─────────┬─────────────┬─────────┬────────────────────┬───────────┬─────────┐
│ (index) │ Task Name   │ ops/sec │ Average Time (ns)  │ Margin    │ Samples │
├─────────┼─────────────┼─────────┼────────────────────┼───────────┼─────────┤
│ 0       │ 'domino''192'   │ 5206131.350000004  │ '±12.86%' │ 20      │
│ 1       │ 'happy-dom''83'    │ 11956762.500000013 │ '±10.99%' │ 10      │
│ 2       │ 'jsdom''57'    │ 17384329.000000015 │ '±3.25%'  │ 10      │
└─────────┴─────────────┴─────────┴────────────────────┴───────────┴─────────┘

Other html parsers could be considered which are faster than happy-dom for this taskare listed here:

@martincizek
Copy link
Collaborator

So I guess the best option for now is to switch to a fork of domino, correct?

@zirkelc
Copy link
Author

zirkelc commented Mar 25, 2024

If speed is the main concern, then yes the domino port passes all tests and is roughly the same speed as the original.

I didn't check the source code of the port, was the issue described in #378 fixed?

If everything is alright, I can cleanup the parser and make the PR ready.

@brianfeister
Copy link

@martincizek what are your thoughts here? I'm trying to evaluate whether there will be a fix for this (I'm working in AWS Lambda) or if I need a different library. That's not meant to be hostile in any way, it's just "hey, have to pivot to keep moving forward"

@brianfeister
Copy link

You could also consider patch-package to address the underlying problem in domino, not sure how long it would be before another issue comes up and you end up indirectly owning that dependency against your preference

@zirkelc
Copy link
Author

zirkelc commented Apr 4, 2024

I checked both the original repo and https://github.com/fgnass/domino and the fork https://github.com/angular/domino for the with() statement that causes the issue described in #378

It this this file in the original repo:
https://github.com/fgnass/domino/blob/12a5f67136a0ac10e3fa1649b8787ba3b309e9a7/lib/sloppy.js#L7-L25

That file doesn't exist in the fork and there is no other usage of this statement. So switching to this should work.

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

Successfully merging this pull request may close these issues.

3 participants