Consider creating unified types for common variables

[jovyntls]

As we continue with the TypeScript migration (#1913), we may want to create unified types to be used across common method arguments. One example could be creating a Node type that is used throughout different methods/classes when the node parameter is passed around.

We'll need to decide:

• When we want to do it — after we have fully migrated, or while the migration is currently underway, or if we even want to standardise these types at all
• What the types should be (e.g. for Node, cheerio.Element & DomElement could be an option)
• How to handle applying types across the whole codebase

[jovyntls]

Some discussion points from #2112 (thank you @raysonkoh and @tlylt):

Potential benefits

• Immutability
• We can have functions to wrap common methods that we call on node - simplifies code and improves code quality

When should we do it?

• Consider waiting until we have more tests in the core package - currently not enough tests to detect potential subtle bugs when refactoring types
• Unifying the types early can simplify the TS migration by reducing the need for undefined/null checks

(How) should we do it?

• cheerio has migrated to TS
• could be related to Investigate any benefits of switching to parse5 library #2075

[ong6]

To continue this discussion, I think that we can set some guidelines for how to handle the types:

Unified Type Definitions

When defining types, we should consider using a type that represents the minimal set of properties and methods required by all the use cases. This helps prevent the types from becoming too specific or cumbersome to use.

Maybe we should create a master thread for all the possible unified types that we can possibly implement once refactoring is done?

When we should do it

I suggest we just wait till we finish the migration and then refactor all types based on the use case (it is more work but will reduce the risk of errors)

How to safely implement this

• Use a code-checking tool, such as TypeScript's linter, to identify any type inconsistencies and ensure that the types are being used correctly.
• Additionally, we can consider using a code generation tool to automatically generate type definitions for common use cases. This can help ensure that the types are consistent and up-to-date throughout the codebase.

[jovyntls]

Another consideration is how we want to organise the types in our codebase. Some options:

• Define the type in an existing file
  • I think we can use this for types that are only used in one file or in a small number of files in the same directory
  • Pros:
    • keeps the type defined closed to where it is used
    • encourages decoupling (discourages using the types outside of this file, because exporting it can make developers double check if they are using it appropriately)
  • Cons:
    • not suitable for types that are used in multiple places across the codebase
    • Also has the possibility of circular imports
• Define types in component.types.ts
  • Similar as above, but separates the types from the file. I think this is a bit cumbersome - all the types are related to a component then they can be defined with the component itself
• Have a types.ts file (example), or types/ directory
  • Could be for types that are used across many directories and files, not tightly coupled with one file
  • I prefer types.ts to a types/ directory - it allows us to separate types without having them too far from where they are used.
  • Pros:
    • Type definitions will all be in one place, or be easily searchable
    • Avoid circular imports - source files import types from type files
    • for types.ts, keeps the types within the same module
  • Cons:
    • Types are defined further from where they are used, even further if we use types/ directory; not necessary for types only used in one file

Currently I'm leaning towards:

• If the type will be used in >1 file (as long as there's export), have a types.ts in the module
• Else, define the type in the file that it is used

[raysonkoh]

I am concerned whether these conventions / standards can be easily enforced by the linter (or maybe some other style check tool) 🤔 . Otherwise, the code base will become even more messy if someone forgets about whichever convention that we decide on in the future.

By the way, TS offers another way for dealing with cyclic imports: https://stackoverflow.com/questions/24444436/circular-type-references-in-typescript , which we could use in the meantime for TS migration.

[jovyntls]

Seeing that the TS migration for most files is completed and we now have a better idea of the types and operations that we perform, I'd like to propose a type wrapper for node:

type Node = DomElement & cheerio.Element & {
  name: string,
  attribs: { [key: string]: any },
  children: Node[],
}

• This new Node type will be introduced as early as possible in NodeProcessor, and all subsequent operations will be done on this type (thus we will not have to deal with DomElement and cheerio.Element directly).
• We can define utility functions for the unavoidable cases where we use a method that requires a typecast to Node, to ensure that these typecasts are done safely. For instance, one such utility function may look like this:

   function parseHTML(html: string) {
       const node = cheerio.parseHTML(html)[0];
       return node as unknown as NodeOrText;
   }

• Finally, to handle cases where we cannot yet be sure that the required attributes for Node exists:

   type TextElement = DomElement;
   type NodeOrText = TextElement | Node;

Rationale

Currently, we

1. Have several as unknown as typecasts, which we're using as a bit of a hack to perform typecasts on incompatible types
2. Have potentially unnecessary on node attributes to satisfy the TS linter. For instance, we often check (if !node.name) return;, which is often unnecessary because node.name should always exist when processing, and if it doesn't, we return early.
3. Convert between cheerio.Element and DomElement almost interchangeably despite these being incompatible types

The above implementation allows us to deal with these issues by

1. Reducing the amount of as unknown as typecasts. Most of these are converting between DomElement and cheerio.Element because we mostly treat them as interchangeable.
2. Enforcing that attributes that we should be present, are always present (e.g. node.name, node.attribs. Thus, we can reduce the amount of unnecessary checks for these attributes.
3. Formalising our view of the relationship between cheerio.Element, DomElement, and all the other attributes that we expect from a node variable. i.e., a node satisfies the type of cheerio.Element and DomElement, and also has a name, attribs (which may be empty), and children (which may be empty).

Additional benefits

• If we decide to switch to the parse5 library, it should be an easier migration since we now have a wrapper type on everything.

A note on immutability: This does not ensure immutability, but also does not hinder any future attempts at having an immutable type (I think). I'm not sure about implementing immutability here, since much of the in the core package is related to transforming nodes. Maybe further discussion is needed on this - e.g., from what point onwards should a node be immutable, or should it be immutable right from the start of parsing?

@MarkBind/active-devs please weigh in on this :)

[raysonkoh]

Thanks for the suggestion @jovyntls, I think this is a good start. My thoughts are that ideally it would be good if updating cheerio or htmlparser2 or switching to parse5 would solve this typing issue. Otherwise, I think your solution might work too, but I think it would be good to trial it with a small PR.

from what point onwards should a node be immutable, or should it be immutable right from the start of parsing?

If we make node (or equivalent) type immutable, any function that tries to manipulate a node obj using side-effects is going to cause a compile error. So the ts compiler would be able to tell us which function needs to be updated.

[jovyntls]

@raysonkoh I suspect that updating cheerio (and/or switching to parse5) wouldn't solve the typing issues since the interfaces remain largely the same. Updating cheerio would allow us to set the parser to use parse5 instead of htmlparser2, but if I'm not wrong, the cheerio methods and classes we use would still have the same typing issues. Same for using parse5 types in place of htmlparser2.DomElement, because much of our typing issues stem from requiring certain attributes from nodes that aren't always in these types.

I could do a small PR to preview the changes for this typing proposal :)

If we make node (or equivalent) type immutable, any function that tries to manipulate a node obj using side-effects is going to cause a compile error. So the ts compiler would be able to tell us which function needs to be updated.

Agreed! I was referring to when we should introduce this type, i.e. at which point should we cast to this immutable Node type. For instance, we could start using it immediately once we do the first parse of the user-supplied markdown, or later on in processNode.

[raysonkoh]

For instance, we could start using it immediately once we do the first parse of the user-supplied markdown, or later on in processNode.

For simplicity and barring any significant performance issues, I think we should go with the former.