Enable strict-mode for TypeScript#1783
Conversation
|
Uhm, apparently draft PRs aren't tested by Travis. |
|
That's OK for now. I'm fixing all the strict errors, so the build would fail anyways. I'll undraft the PR once it's in a buildable state on my local. |
|
The changes are coming along well. The main hurdle I need to tackle is the types returned for JSX. In the context of dom-chef, we don't want the return type to be JSX types, we want it to be that particular element. e.g. |
|
I'll get back on this in the next couple of days. Been under the weather a bit. 🤒 |
|
@bfred-it @sindresorhus, we are green. |
|
I think we're good here aside from another PR to put types that belong in their respective repos. Also, just want to make sure you're good with the WeakMap as suggested @bfred-it, https://github.com/sindresorhus/refined-github/pull/1783/files#diff-7f45906e508cad55f59cba1e4cf34ceaR13 |
|
@bfred-it I think we should merge this now if it doesn't break anything. We can continue commenting after merge and that can be fixed in a follow-up PR if there's anything. |
|
Let's test it out for a few hours and then I'll merge it (in ~10 hours?) |
fregante
left a comment
There was a problem hiding this comment.
Just sorting out some compilation issues. I fixed the background entry point, but the options entry point still errors out when running
npx tsc ./source/options.tsx.npx tsc ./source/options.tsx node_modules/indent-textarea/index.d.ts:3:9 - error TS2502: 'watch' is referenced directly or indirectly in its own type annotation. 3 var watch: typeof watch; ~~~~~ Found 1 error.From ... #1783 (comment)
Fixed in the latest indent-textarea. I saw this issue in many of my recent modules and they have all been fixed. https://github.com/bfred-it/indent-textarea/releases/tag/v1.0.4
source/features/mark-unread.tsx
Outdated
| </a> | ||
| ); | ||
|
|
||
| const renderIcon = type === 'issue' ? stateIcons.issue[state as IssueNotificationState] : stateIcons['pull-request'][state]; |
There was a problem hiding this comment.
See ba30b4b
I prefer one line of dead code instead of lengthy types — same solution in another part of the same file: https://github.com/sindresorhus/refined-github/blob/cd741d368446f1d71877a00a74c84e84f9e488e3/source/features/mark-unread.tsx#L106-L114
| const linkedSourceDom = Symbol('Attached RGH source dom'); | ||
| const linkedRenderedDom = Symbol('Attached RGH rendered dom'); | ||
| const rghButtons = new WeakMap<Symbol, Element>(); | ||
| const rghButtons = new WeakMap<Element, Element>(); |
|
Two months in the making, probably the biggest PR in Refined GitHub 🗿 Great work @nickytonline! |
|
Amazing work, @nickytonline and @bfred-it ✨ |
| } | ||
|
|
||
| // TODO: Move types to the https://github.com/sindresorhus/linkify-issues repository. | ||
| declare module 'linkify-issues' { |
There was a problem hiding this comment.
linkify-issues and linkify-urls now ship with their own TS types, so these can be removed.
Note that the repo option in linkify-issues was renamed to repository.
There was a problem hiding this comment.
Sounds good. I'll remove the types.
Basically going with the non-null assertion operator in almost all places, because as mentioned in #1735 (comment), the assumption with the extension is that the DOM nodes referenced in most cases are expected to be there. If they aren't, the extension breaks, a bug is filed and it's fixed.
One thing to note @bfred-it, is currently dom-chef types need to handle JSX (Element and IntrinsicElements) or we import
@types/react.Closes #1735