Skip to content

Conversation

@fb55
Copy link
Collaborator

@fb55 fb55 commented Apr 13, 2023

entities@4.5.0 features an EntityDecoder that implements the entity decoding logic from this module in a way that can be shared between my three projects working with HTML entities (other PRs: fb55/entities#1136, fb55/htmlparser2#1480).

With this change, the main use-case for hibernation is removed. The remaining use, waiting for sequences, can be removed in a future patch, which should make it easier to follow the logic in the tokenizer (I was surprised several times by unintuitive flows while working on this PR).

Ultimately, I'd like to see both htmlparser2 and parse5 become more modular, which will make them easier to maintain, and should provide a big speed boost — https://github.com/marko-js/htmljs-parser is currently the fastest parser in https://github.com/AndreasMadsen/htmlparser-benchmark, using a modular design to great effect.

"preset": "ts-jest/presets/default-esm",
"testEnvironment": "node",
"coverageProvider": "v8",
"globals": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change, but this removes a deprecation warning from ts-jest. There is no change in behavior.

@fb55
Copy link
Collaborator Author

fb55 commented Apr 24, 2023

@wooorm @43081j I'd love your review if you are available!

@wooorm
Copy link
Collaborator

wooorm commented Apr 24, 2023

Sorry, I have no knowledge of the entities package, or deep knowledge of the internals here, to judge this PR.

I am not sure about your point “The remaining use, waiting for sequences, can be removed in a future patch”. Why is it a good thing to remove that? I personally use hibernation to pass different sequences through.

@43081j
Copy link
Collaborator

43081j commented Apr 24, 2023

I'll have a read through 👍

Copy link
Collaborator

@43081j 43081j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry it took so long, i've been away recently

looks good to me, though i haven't had chance to read much into the entities package in your repo

@fb55 fb55 merged commit 538f592 into master May 8, 2023
@fb55 fb55 deleted the entity-decoder branch May 8, 2023 09:54
@fb55
Copy link
Collaborator Author

fb55 commented May 8, 2023

Thanks for the review!

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.

4 participants