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

PR: backports parse6 bom_fix for cheerio issue #349

Closed
frank-dspeed opened this issue Jul 1, 2021 · 5 comments
Closed

PR: backports parse6 bom_fix for cheerio issue #349

frank-dspeed opened this issue Jul 1, 2021 · 5 comments

Comments

@frank-dspeed
Copy link

frank-dspeed commented Jul 1, 2021

@43081j
Copy link
Collaborator

43081j commented Jul 11, 2021

"parse6"? have you made a fork? have you opened wider discussion in an issue anywhere for that?

would be worthwhile understanding what lead to you making a fork and why you couldn't contribute it to the official package.

i ask because i've also got a branch where i've moved parse5 to ESM, with the aim of moving it to typescript after that. but HTML parsing is a messy job, so if you're already working on something please do open it for wider discussion.

(sorry to anyone else for hijacking the issue)

@frank-dspeed
Copy link
Author

@43081j #330

@frank-dspeed
Copy link
Author

i am not sure if i know your ESM port alreadyi saw some but my rewrite is far more deep and i break a lot of the existing API.

because i do want to have it without "this" usage and a lot of more to also run it in production.

I already have full typescript support but i am not using .ts extension.

While my rewrite is really far and did correct a lot of internal messy stuff like the state tracking it is to early to publish it as publishing would force me to keep the existing api.

And as sayed i break the whole api and did also rewrite the test harness.

as i started with the refactoring i found all kind of small logic failures that simply worked because of lucky conditions.

my main goal is to get the code readable for some one who never saw it before is maybe the most importent step.

@frank-dspeed
Copy link
Author

@43081j oh and i want to tell you why i do not directly contribute here i did want to do so but i feel bad this has millions of users i can never break the api of this repo. so it was logical that a fork is needed

@fb55
Copy link
Collaborator

fb55 commented Jan 13, 2022

As discussed in #350, it unfortunately doesn't seem like this is the right way to go about things.

@fb55 fb55 closed this as completed Jan 13, 2022
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 a pull request may close this issue.

3 participants