feat(TS): migrate to TypeScript #76
Conversation
src/index.tsx
Outdated
| | ErrorBoundaryPropsWithRender | ||
|
|
||
| type ErrorBoundaryState = {error: Error | null} | ||
| const initialState = {error: null} |
There was a problem hiding this comment.
👋 you need to specify:
const initialState: ErrorBoundaryState = { error: null }Otherwise the typeof state = initialState will be { error: null }. This is what the rule @typescript-eslint/no-unnecessary-condition is complaining about, error !== null is unnecessary because it's typed as error: null so from TS perspective error !== null will never be true.
This is because of the generic {error: Error | null}. Since { error: null } extends ErrorBoundaryState then it's inferred as { error: null } when you do state = initialState.
Alternatively, you could set in line 70:
state: ErrorBoundaryState = initialStateAnd then you'll be able to remove the comments regarding @typescript-eslint/no-unnecessary-condition
I hope this makes sense 😅
There was a problem hiding this comment.
Makes perfect sense. And it works. Thank you!
.github/workflows/validate.yml
Outdated
| needs: main | ||
| runs-on: ubuntu-latest | ||
| if: | ||
| ${{ github.repository == 'kentcdodds/react-error-boundary' && |
There was a problem hiding this comment.
Shouldn't this be bvaughn/react-error-boundary? 🤔
There was a problem hiding this comment.
Yup! Thank you.
src/index.tsx
Outdated
| const initialState = {error: null} | ||
| class ErrorBoundary extends React.Component< | ||
| // TODO: why is there not a PropsWithRefAndChildren? 🤔 | ||
| React.PropsWithRef<ErrorBoundaryProps>, |
There was a problem hiding this comment.
Maybe?:
React.PropsWithRef<React.PropsWithChildren<ErrorBoundaryProps>>,😅
There was a problem hiding this comment.
Makes sense 😆 Thanks!
| @@ -0,0 +1,128 @@ | |||
| # Contributor Covenant Code of Conduct | |||
There was a problem hiding this comment.
Did you guys discuss the move from https://github.com/bvaughn/react-error-boundary/blob/master/other/CODE_OF_CONDUCT.md (http://contributor-covenant.org/version/1/4) to this version (https://www.contributor-covenant.org/version/2/0/)? The commit message makes it sound like the change was accidental...
There was a problem hiding this comment.
I've been slowly upgrading all of my OSS projects to the latest version.
| @@ -39,20 +39,22 @@ test('handleError forwards along async errors', async () => { | |||
|
|
|||
| await screen.findByRole('alert') | |||
|
|
|||
| const [[actualError], [componentStack]] = console.error.mock.calls | |||
| const firstLineOfError = firstLine(actualError) | |||
| const consoleError = console.error as jest.Mock<void, unknown[]> | |||
src/index.tsx
Outdated
| const props = ({ | ||
| error, | ||
| resetErrorBoundary: this.resetErrorBoundary, | ||
| } as unknown) as FallbackProps |
There was a problem hiding this comment.
I don't get why you need the type cast if the shape looks correct...
There was a problem hiding this comment.
I can't remember why, but it complained about it at one point. I don't like type casting at all, but I wanted to get it committed 😅
Removing it now, and it's fine with it as-is so 🎉
src/index.tsx
Outdated
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| if (error !== null && changedArray(prevProps.resetKeys, resetKeys)) { | ||
| this.props.onResetKeysChange?.( | ||
| prevProps.resetKeys as Array<unknown>, |
There was a problem hiding this comment.
I wonder if making type generic in the type of reset keys (likeArray<T>) would help with type casting.
I'm of the opinion those should be used sparingly.
There was a problem hiding this comment.
Actually, I figured out another way to fix this that didn't require type casting 👍
|
Not sure why the snapshots are failing 🤔 Will look into it later 😬 |
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 40 48 +8
Branches 11 11
=========================================
+ Hits 40 48 +8
Continue to review full report at Codecov.
|
|
Looks like the snapshot issue was related to Node 10. I don't care all that much to make sure the build runs on Node 10, so I just removed it. |
|
🎉 This PR is included in version 3.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What: Migrate to TypeScript
Why: To have much better type definitions and improved confidence
How: upgrade all deps and leverage the new TS features from kcd-scripts
Checklist: