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

Revamped package #12

Open
fb55 opened this issue Nov 1, 2021 · 5 comments
Open

Revamped package #12

fb55 opened this issue Nov 1, 2021 · 5 comments

Comments

@fb55
Copy link

fb55 commented Nov 1, 2021

Hi @domenic et al,

I was looking into using this package for cheerio, but had several issues that I wanted to fix first. As extending the current html-encoding-sniffer package turned out to be cumbersome, I opted to write a new module instead:

https://github.com/fb55/encoding-sniffer

This new package implements the current version of the encoding sniffing algo as a state machine. That allows streams to be supported without much effort. Features this supports, which aren't present in html-encoding-sniffer:

I would love to join forces and have a single package that both jsdom and cheerio can use going forward. Let me know if this is something you'd be interested in!

@domenic
Copy link
Member

domenic commented Nov 1, 2021

Interesting.

XML encoding types (UTF-16 prefixes and <?xml encoding="...">)

I believe these (at least the latter; not sure about the former) were only recently added to the HTML spec. I'd love to add support for them to html-encoding-sniffer, following the spec at https://html.spec.whatwg.org/multipage/parsing.html#prescan-a-byte-stream-to-determine-its-encoding

Configurable sniff depth (see #10)

Seems reasonable, although I'm not clear on the use case, since AFAIK all browsers use 1024.

x-user-defined in <meta> tags (turns out html-encoding-sniffer's support is broken)

This is not broken; it's following the spec: https://html.spec.whatwg.org/multipage/parsing.html#prescan-a-byte-stream-to-determine-its-encoding step 4.case2.15.

Streams / sniffing of incomplete documents

I'm hesitant to add the complexity of streams (especially Node streams) to this package, so this would require some extra discussion as to how we could layer the API into a non-streaming version and a streaming version.

@fb55
Copy link
Author

fb55 commented Nov 1, 2021

x-user-defined in <meta> tags (turns out html-encoding-sniffer's support is broken)

This is not broken; it's following the spec: html.spec.whatwg.org/multipage/parsing.html#prescan-a-byte-stream-to-determine-its-encoding step 4.case2.15.

whatwgEncoding.labelToName doesn't return a value when passing x-user-defined. So the branch will never be hit.

@fb55
Copy link
Author

fb55 commented Nov 1, 2021

To be clearer about what I'm proposing:

I would like https://github.com/fb55/encoding-sniffer to become html-encoding-sniffer@4.0.0. This will include API compatibility & all existing tests will be added to the new project. Repo ownership TBD, it can live in the JSDOM org.

@domenic
Copy link
Member

domenic commented Nov 1, 2021

whatwgEncoding.labelToName doesn't return a value when passing x-user-defined. So the branch will never be hit.

Ah, I see, thanks.

I would like https://github.com/fb55/encoding-sniffer to become html-encoding-sniffer@4.0.0.

Oh, sorry, no, I am not interested in that. I thought you were offering to improve html-encoding-sniffer.

@fb55
Copy link
Author

fb55 commented Nov 1, 2021

Oh, sorry, no, I am not interested in that. I thought you were offering to improve html-encoding-sniffer.

No worries, thanks for the reply. Just in case the semantic difference matters to you: I am also happy to open a PR that adds the state machine of fb55/encoding-sniffer to this package, and archive my repo.

The original wording was supposed to cover both, and I honestly just want to have a package that covers the use-cases I have for cheerio.

domenic added a commit to jsdom/whatwg-encoding that referenced this issue Nov 12, 2023
domenic added a commit to jsdom/whatwg-encoding that referenced this issue Nov 12, 2023
domenic added a commit that referenced this issue Nov 12, 2023
domenic added a commit that referenced this issue Nov 12, 2023
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

No branches or pull requests

2 participants