Refined-github: Convert the whole codebase to TypeScript and make it strict

[sindresorhus]

• [x] Convert the remaining files to TypeScript:
  https://github.com/sindresorhus/refined-github/tree/master/source/libs
  https://github.com/sindresorhus/refined-github/tree/master/source
• [x] Enable strict mode: https://github.com/sindresorhus/refined-github/blob/11b927189bab258a226e251c1bd87504feee51f4/tsconfig.json#L10
• [ ] Enable these rules again and fix the lint errors: https://github.com/sindresorhus/refined-github/blob/e90b58c16f58f97a0758de05ae7fe01bf7f65687/package.json#L77-L78

[mohamedmansour]

@sindresorhus why did you name them .tsx file? and not .ts ?

[hansott]

@mohamedmansour Some files contain JSX, e.g. https://github.com/sindresorhus/refined-github/blob/master/source/features/trending-menu-item.tsx

[sindresorhus]

https://www.typescriptlang.org/docs/handbook/jsx.html

[mohamedmansour]

I Didn't see some files have it, majority do not. Okay :)

[nickytonline]

I can grab this. I'm pretty comfortable in TypeScript land.

[sindresorhus]

@nicksnyder Awesome! Go ahead 🎉

[nicksnyder]

Wrong Nick :)

[Raika]

I can help too.
where should I start?

[nickytonline]

@sindresorhus @bfred-it, I've started the next step to make things strict. I just wanted to check with both of you how you see the strictness being implemented.

There's lots of examples in the codebase that always assumes that something exists, e.g.

const commentedNode = el.parentNode.nextSibling as Text;

There are a few ways to approach this:

// Short and sweet with the non-null assertion operator
const commentedNode = el!.parentNode!.nextSibling as Text;

or

// Can kill readbility
const commentedNode = (el as Element).(parentNode as Element).nextSibling as Text;

or

// Lots of boileplate for existence checks but better guarantees
if (!(el.parentNode && el.parentNode.nextsibling)) {
    return;
}

const commentedNode = el.parentNode.nextSibling as Text;

• The first way reads well and we just tell TS we know it exists
• Second way is potentially harder to read if properties are deeply nested and we just tell TS we know it exists.
• Third way can get more verbose with all the existence checks, but is more robust.

I try not to cast too much or use ! too often, but ! can have value in terms of better readability especially in a case like RGH where a lot of DOM elements are assumed to exist.

The other thing is as soon as strict mode is enabled, it complains about JSX. From what I can tell, the only way to type it so the compiler doesn't complain is to cast, e.g.

((<svg aria-hidden="true" className="octicon octicon-check" width="12" height="16"><path d="M12 5l-8 8-4-4 1.5-1.5L4 10l6.5-6.5z"/></svg> as unknown) as SVGElement);

Also, dom-chef has no types and since it's React like in terms of JSX, I brought in the @types/react package. The other thing it complains about is all the class attributes on the JSX. dom-chef supports className, so I'd suggest going with that.

The above double casting can become a lot of copy paste as there is a lot of JSX in the project. One other option could be an identity function that just returns it cast properly. It means a useless function to just get the right type, but it may read better. Just a suggestion.

function jsxToElementType<TElement extends Element = Element>(jsx: unknown) {
    return jsx as TElement;
}

Let me know what you think. I'm going to catch some 💤

[fregante]

Thanks for checking first! Honestly I don't like any of that. Can we avoid the strictness altogether?

Sometimes we need to check each element (e.g. if !el, return) because the page filter (e.g. isRepo) isn't as granular as needed; most of the times though _we need_ it to fail and error out if the DOM doesn't match our expectations, so someone will notice quicker and it will be fixed, instead of silently failing.

[nickytonline]

We can avoid the strictness altogether, it's just that you can't enable strict mode then (one of the criteria in this issue). So if you still want to enable strict mode, I'd suggest using the non-null assertion operator, !, based on your comments.

most of the times though _we need_ it to fail and error out if the DOM doesn't match our expectations, so someone will notice quicker and it will be fixed, instead of silently failing.

[nickytonline]

@bfred-it, just following up on this. Are we just not going to enable strict mode then?

[fregante]

@sindresorhus

[fregante]

Perhaps open a PR with what you have

[sindresorhus]

@bfred-it The ! operator fixes most of these annoyances. I use it heavily when I know it's not going to be null. Strictness doesn't have to be all or nothing either. We can choose to enable just some of the strictness flags.

[hardikmodha]

Is anybody still working on this? I would more than happy to pick this up.

[sindresorhus]

@HardikModha It's being worked on in https://github.com/sindresorhus/refined-github/pull/1783.