-
-
Notifications
You must be signed in to change notification settings - Fork 232
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: 7.0 - ESM - Only version #330
Comments
@frank-dspeed Is there anything that can be done to move this work forward? I am about to fork the htmlparser2 tree adapter to resolve #327, and would love it if this could be resolved in the main module instead. |
@fb55 this is a big project that contains a lot of small bugs and needs complet rewrite i will release it as parse6 and will also make sure that html6 is handled correct it makes no sense to move this codebase forward sorry to say that. I have rewriten the whole script here 8 times i know exactly what i am saying. Some Problems with the existing Codebase
so overall refactoring would end in a diffrent API so introduce so much changes that a new name is needed or all maintainers would need to agree on same principles The biggest problem is really also the test framework as i recoded this i also needed to refactor the whole test Harness. |
@frank-dspeed Thanks for the update! Looking forward to parse6, and curious to see how the new code will be structured. Regarding parser adapters: I do think they provide a lot of value, and the biggest issue of the current parse5 project is testing at the wrong abstraction level. To make things a bit easier, I'm happy to provide an interface for your parser from the Please let me know if there is anything I can help with! |
see this #351 tests pass, seems to be stable. i don't think you need all the extra "nice to have" refactorings to at least move to ESM. in that PR it is already done and was fairly straight forward. the complexities here are more around refactoring legacy logic IMO, things that now can be written in a nicer way with features which didn't exist at the time parse5 was written. i also don't think it makes sense by name to be pushing a 'parse6'. by all means throw whatever fork together you want, but it isn't version 6 of parse5, its a different project by a different maintainer. the 5 also refers to HTML5 (of which there isn't a 6th yet, and when there is a parse6 would make sense, but only then). i've written most of the types for parse5 and dealt with it a fair amount, so if you are open to feedback before going off and throwing together a fork, i'd be happy to help. |
@43081j sure it is a diffrent project with a diffrent maintainer it has total diffrent goals i only call it parse 6 because i also wanted something that is editable to add upcoming html6 spec |
to be clear, im not opposed to a fork. inikulin hasn't seemed so active these days so it may be the right path to go down. but i definitely wouldn't claim the "parse6" name. it seems like bikeshedding but it implies it is a progression from parse5, and we really should save the parse6 name for a HTML6-compatible parse5-like library. if your aim was to only support html6 (when it exists), it'd make some sense. but it isn't right now, its a fork of parse5, still a parser for HTML5, not a new version of parse5. other than the name, a fork may be a good idea to clean up a fairly inactive project. its likely ill also do a typescript build follow up from my ESM PR, but im happy to throw it all away if your fork progresses (under a new name) to a useable state. it'll still be useful to give you feedback from my own findings |
@43081j i see parse6 logical even today as parse 5 is ES5 and we did progress to ES6+ what do you think? and then there will be also html6 |
it is named parse5 because it parses HTML5. i would assume parse6 is a parser for HTML6. any fork anyone makes also isn't a newer version of parse5, it is a fork. i just think it would be better off under its own name to avoid confusion, not to lead people to believe it is by the same maintainers etc. as for contributing back to the repo, IMO inikulin should really be open to a breaking change PR to move this project forward. but if he doesn't have the time or interest in this project anymore, a fork would make more sense |
@frank-dspeed @43081j Awesome to see you two looking to move this project forward! @inikulin seems open to adding contributors, and has offered to do so in #327. I have just followed up with him, to see if this can happen soon. I agree that breaking changes are very much doable within the I actually couldn't find anything about a WHATWG HTML6 — @frank-dspeed would you happen to have a link? |
fwiw my original plan before i realised others were doing similar work:
i don't believe the right thing to do is fork it and rename it if inikulin has already said he is looking for contributors. these could easily be done in this existing repo and @inikulin could add trusted contributors with write access. no need for another org, or another package. |
@43081j its all to late and fb55 did not accept fast so i will tell you what currently gets done. I Fork Cheerio, JSDOM, Parse5 and all the related Modules of them and create 1 clean codebase. The most hard part was to decouple all the Stuff as it is connected in places where you would not expect that they are connected. The most big change out of Advanced user view will be the EventEmitter Pattern i have a minimal event emitter pattern included that works on the web and inside Nodejs. I hate when some one overwrites existing functions on a Object so i Implemented the option to register callbacks via on for the relevant parts. Also i refactored the stream stuff to iterators that can get used via a 1 liner to create a nodeJs stream. sure i am doing at present more but that is what the user will see. @43081j oh and i looked into your fork deeply the only thing that survived from it was the change to the "tdd ui" but that was already a massiv help Thanks for that. Update#327 (comment) fb55 accepted and wants to merge this already into cheerio so you see as i want to have also cheerio and JSDOM in Clean ESM without Build but with TypeScript support without using .ts extension. It is the right way to create a new Project with cheerio and parse5 as also JSDOM |
To clarify: I don't want to merge parse5 into cheerio. I would like to move the project to an org, which will make it easier to manage contributors, create repos for sub-projects and potentially look for funding. The @cheeriojs org already has a lot of this set up and feels like a natural home for parse5 to me. If @inikulin doesn't feel the same way, having a dedicated org is a great option as well. Regarding a fork: @43081j @frank-dspeed In the meantime, we should decide if we are going to work on this together. If so, let's try to have a joint vision of where this library should go. |
@fb55 I am with you as long as
When you agree on all that we are on the same page if you disagree with any point for any reason I need to fork it independently. In general, I am really happy with my new version I found a lot of issues and learned a lot about converting a huge class-based NodeJS Project to ESM. |
As NodeJS LTS Now Reached the state where ESM Works and we got a 100% Way to shim it for older NodeJS Versions it
is time to Refactor this to ESM Only which would directly run Inside Nodejs & WebBrowsers
Issus to Tackle in this Major
Old Issue that got noisy and out of topic discussions
TODO
5th iteration conclusions
Findings:
The Result of new Parser() should get wrapped when extending the parser it is not so easy to follow the inharitance of objects with
the self written Mixin Class system that offers a later install method which will patch a eventual existing _bootstrap method that later
gets eventual called when using Parser.parse or Parser.parseFragment.
All extensions should be written as proxy the treeAdapter should be named NodeAdapter is it is not forming the tree out of browser view
it is a Node api so we should express it like that to make more clear for other developers that there is the place to manipulate Node
Creation and handling. Proxy support starts with Nodejs 6.4 shim https://github.com/GoogleChrome/proxy-polyfill/blob/master/src/proxy.js
on the same sharedState Object that got passed to the hireOrderFunction
I Think .js JSDOC import is better as it supports to even bundle the TypeDefintions)
8ef28cd
Lexer implementation started. 0.6.1 … master v6.0.1 Ivan Nikulin committed on 15 Mar 2013
Why ESM Only?
my final Way to go will be a ESM only version this can then get transpiled via babel to cjs or with anything else.
Then the ESM version gets released as new Major version and can get used with current NodeJS via import() inside CJS
the README will show instructions to create a CJS version if needed. And it will point out that this new Major Version is Only ESM and to use it you will need NodeJS Version > 9.7.x or the npm esm package.
What do you think about it?
Browser ESM Support
NodeJS ESM Support
NodeJS & BrowserProxy Support
The text was updated successfully, but these errors were encountered: