Next steps

[fb55]

Now that we have forked, what should happen?

Things I'd like to see:

• Port to TypeScript
  • This might require mixins to be merged into the main modules.
• Modern JS
  • Introduce Maps and Sets where appropriate
  • Use new methods, eg. includes, startsWith
  • Use property shorthands
• Use the latest HTML5 tests
  • Move away from @inikulin's fork: https://github.com/HTMLParseErrorWG/html5lib-tests/

[43081j]

sorry it took a while to get to this.

since you and i both converted the repo to typescript at the same time, i think a sensible thing to do would be to compare implementations roughly and adopt any improvements from mine into this fork. im happy to then discard the work i've done moving it to typescript, as long as we're both happy this one is ahead of it/better.

i think it'd make sense to also convert all the mixins at some point into plugins instead. this would need discussion though and a rough spec of how it'd work (so the parser would call its plugins rather than mixins mutating the parser, etc).

what'd be very useful from you is to document some kind of overview of what you've done here. it isn't only an esm/typescript port, you've also changed quite a lot of things. i'd like to understand those changes and the differences in this repo.

i'd be happy to then collaborate on this fork and PR into it rather than parse5. if you're around on twitter it may be worth having a conversation over DM so we can compare thoughts etc.

[43081j]

if you can also re-invite me to the org, that'd be appreciated. it seems the link i dug out no longer works.

[fb55]

Sent again! Also pinged you on Twitter, happy to chat there as well.

As for changes: The two big changes not listed above were breaking the switching maps of the tokenizer and the parser, and using if/else blocks instead. This was from experiments when working on htmlparser2, and turned out to lead to a nice speed improvement here as well.

Besides that, I:

• Switched the order of the formatted elements list, which helped with:
• Introduced array helpers in a lot of places
• Switched to Jest as the test runner
• Updated tests to no longer build objects of tests (used describe/it constructs instead)
• Enabled several ESLint rules for modern JS, eg. object shorthands, template literals, arrow functions

[43081j]

i'll comment my findings as individual posts so we can reply to each separately

[43081j]

typescript-eslint recommend config & strict compilation

All strict options in typescript enabled, along with the @typescript-eslint/recommended eslint rule set being extended.

This means no more any, non-null-assertions (! token) and every method/function having a strongly defined return type. A lot of this codebase is missing those things.

[fb55]

I agree with return types, and have attempted to add them in most places; same applies to private/protected/public. Same for any.

About non-null assertions: I do like having them in some places where TS just isn't able to infer correct types. Eg. take

if (arr.length > 0) let foo = arr.pop();

foo will be possibly undefined, even though we know that isn't the case. I agree that we should restrict how much we use them. One way would be to get rid of _bootstrap.

[43081j]

most of the places we use them now are around bootstrapping, which kinda makes sense but on the assumption the consumer has used the class perfectly. IMO it would be nicer to account for nulls either by throwing a more helpful error or by returning early. the non-null assertions mean we don't account for any of this so would just end up with null exceptions if someone called methods in the wrong order (very possible).

isn't terrible to have them though so its your call. somehow moving off bootstrap() would be nice for sure, constructing as much as we can up front in the constructor would be ideal

[43081j]

tree adapters are now classes

I reworked tree adapters to be classes which conform to an interface, rather than importing the module and hoping for the best.

You can see the interface/types here.

The default tree adapter here.

Currently, your tree adapters don't implement the interface really. there's no way inside the default tree adapter file to get type checking on if we've implemented the tree adapter interface correctly. for that and other reasons it should be a single object exported which is strongly typed as the interface.

[43081j]

comments on your tree adapter interface

I don't think the type map should be generic the way you've done it. we shouldn't be restricting tree adapters to ensure everything inherits their Node for example.

this also doesn't make the most sense with the parse function. parse<T extends TreeAdapterTypeMap> is the same as saying parse<T extends TreeAdapterTypeMap<{}, ...>. which isn't necessarily true, a tree adapter type map might define their node as not extending {} (object) technically.

the interface was specifically not generic before for these reasons.

getNodeSourceCodeLocation can return a Location, not only an ElementLocation. it is meant to work with any node.

setNodeSourceCodeLocation should accept any Location too.

it is up to mixins and tree adapters if they want to restrict it to elements.

updateNodeSourceCodeLocation doesn't consume a partial element location. it is specifically an end location, optionally with tag location:

    updateNodeSourceCodeLocation(node: T['element'], endLocation: EndLocation & { endTag?: Location }): void;
    updateNodeSourceCodeLocation(node: T['node'], endLocation: EndLocation): void;

i used overloads to deal with this. a node can never have an endTag location, but an element can. otherwise you'll end up weakening the type here to accept endTag for nodes.

[43081j]

comments on your default tree adapter / node interfaces

In my branch, i make heavy use of tagged unions. This makes it very easy to narrow types of node to the right type.

with your current interfaces, it'll be a real pain for consumers to deal with the produced AST. lots of casting will be needed, which was a problem with parse5 already until i partially fixed it in the DT types.

For example, using your code:

declare const node: Node;

if (node.type === 'comment') {
  // annoyingly still a `Node`, no use to me without a cast
}

with my branch, typescript will infer that it is a CommentNode inside the if block.

i'd highly recommend we do this in this fork, as it is already a real pain in the existing parse5 types i've spent plenty of time trying to improve.

you can see how it is done here.

You should still have a BaseNode or something, and your NodeWithChildren, NodeWithParent but nothing would be expected to consume those. Similarly, i'd argue most or all of your methods shouldn't consume these, and should consume Node where possible.

For example:

export function detachNode(node: NodeWithParent) {
    if (node.parentNode) {

parse5 checks if there's a parentNode because the parameter was always intended to be Node, not specifically nodes with parents.

similar, appending children would ideally consume a Node as the child to append. consumers won't always have a NodeWithParent/NodeWithChildren, and are more likely to have a Node. i'm aware of many places (in packages consuming parse5) where we append a Node.

[fb55]

Agreed. I copied the node structure that I implemented in htmlparser2, and the proposed structure is a clear improvement.

[43081j]

comments on locations

as mentioned in the other posts, updateNodeSourceCodeLocation is meant to be called with an EndLocation.

the two call-sites are here and here.

this is why I have an EndLocation and StartLocation, which Location is a union of.

you can see here.

strangely, your ElementLocation is alongside the tree-adapter interface, separate from all the other location interfaces. maybe that should live with the rest of the location stuff.

[fb55]

I had locations implemented as a StartLocation and an EndLocation for a bit. I decided for the partial for updateNodeSourceCodeLocation, as it is also called with a endTag location. Also, the partial is in line with the actually implemented behaviour, and would allow for other updates as well.

strangely, your ElementLocation is alongside the tree-adapter interface, separate from all the other location interfaces. maybe that should live with the rest of the location stuff.

Good point, moved this.

[43081j]

updateNodeSourceCodeLocation is called with one of two things:

• what i'm calling an EndLocation (the end parts of a Location, so endCol, endOffset, etc)
• an EndLocation with an endTag

both are end locations, but one has an extra endTag property.

that's why i had it with two overloads (i commented that in another post in here):

    updateNodeSourceCodeLocation(node: T['element'], endLocation: EndLocation & { endTag?: Location }): void;
    updateNodeSourceCodeLocation(node: T['node'], endLocation: EndLocation): void;

this'd force consumers to know for sure they have a T['element'] if they want to set an endTag location. otherwise, they can only set an EndLocation.

importantly, its not a Partial<ElementLocation>, its specifically the end of that, i.e. we shouldn't be able to pass in {startOffset: 10} but can right now

[43081j]

mixin utility

The mixin util types are fairly weak and not quite right, it would be good to strengthen them.

Examples:

parse5-fork/packages/parse5/lib/utils/mixin.ts

Line 14 in fe68cf6

protected abstract _getOverriddenMethods(mixin: Mixin<Host>, originalMethods: Host): Partial<Host>;

originalMethods is not a Host, it is actually a partial of it, but specifically the methods you've overridden. This isn't straight forward to type, so i ended up with Partial<THost> and had to use null checks when calling into it (host.someMethod?.call(this, foo)).

Also, if you use a for...in, you'll get better type inference than Object.keys.

You can see what i did here.

With mine, i did have to specify the type of this where you consume:

https://github.com/43081j/parse5/blob/a4052dfd9962e29d43ec6a2c771d9736d399e358/packages/parse5/src/extensions/location-info/open-element-stack-mixin.ts#L22

The ideal would be to strongly type the parameter as the same type as the return, but i think time is better spent throwing this whole idea of mixins away and building a plugins system instead.

[fb55]

This is a good point, and using for ... in is a good call — thankfully mixins are gone, and I can ignore this 😄

[43081j]

open element stack

its in the name: open element stack. we should change the type of its items and current properties to be T['element']. there's no logical way to add a non-element to the stack, already.

[fb55]

The root node of the stack is the document, which is not an element. We can definitely revisit this, but at the moment this would be a bigger refactor.

[43081j]

the current is initially a document, yes, but items never contains anything other than T['element'].

in fact, almost all the loops over items assume it is an element. for example, they call things like getTagName(items[i]), which im surprised compiles 🤔 since items[i] would be a ParentNode but getTagName takes an Element... not sure how that worked 😂

i suggest we have current: T['element']|T['document'] and items: T['element'][] at least.

i really wanna know why that getTagName compiles now... 😬

[43081j]

yeah something's up with the type map type. probably related to my post about that:

OpenElementStack<T extends TreeAdapterTypeMap<{}, {}, {}, {}, {}, {}, {}, {}, {}, {}>>.current: T["parentNode"]

is what it resolves to inside the open element stack. so i think typescript is seeing T['element'] as being {}, which T['parentNode'] has also resolved to... meaning it thinks passing a T['parentNode'] to something which only consumes T['element'] is fine. lets discuss in the other post about that type though i think

using my type map from my branch also results in the same! maybe because in mine, everything narrows to unknown 🤔 guess we need to think of a way to make this map strongly typed...

[43081j]

mixins in general

Again i think they should go away, but anyway....

I noticed in many places, you cast the mixins as any. This smells like a hack, so the types should be corrected/improved to make it work without the cast (maybe the post above about the mixin util will sort this).

Also, most of the methods the mixins override are meant to be protected. once we add visibility to methods, the mixins will (or should) fail to build. In my branch, i ended up making a whole bunch of methods public to work with this. Really, though, we should mixin over typeof this rather than the public facing class i suppose (allowing access to protected/private members). or just make them public like i did.

[fb55]

Finally starting to reply, and this one is easy: I removed Mixins yesterday, with a small perf penalty. Most of the code folded nicely into the existing classes, and I was able to remove a lot of the hacks that were previously in place.

[43081j]

do you have a PR or commit you can point me to for that? would be good to see how it works the new way

[fb55]

Unfortunately this was a number of commits, starting with 3ab6403. Others are 758f13c, 54e1feb, 269ef1c, b82e3e9 and 1e3643e.

I will get in the habit of opening PRs for streams of work again. Directly committing to master is great for stream-of-consciousness style working, but we are now at a point where that is no longer helpful.

[43081j]

no worries, i'll figure it out and have a read 👍 and definitely, working against PRs from now on would be a lot more helpful for collab