test: setup TypeScript settings #824
Conversation
| @@ -1,12 +1,7 @@ | |||
| let React; | |||
| let ChildMapping; | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Not really a fan of TypeScript in runtime tests precisely because runtime tests are annoying to type. The previous test did work differently by clearing modules for each test. A static import is not the same.
If you want to test the types, we should have separate test files for them.
There was a problem hiding this comment.
The previous test did work differently by clearing modules for each test. A static import is not the same.
Yeah, but this seems not to be necessary because the tests don't mock anything or mutate modules. I prefer not to mock or mutate modules as possible. So it would be nice to use static imports as default.
If you want to test the types, we should have separate test files for them.
That is a valid opinion, but I'd like to write tests in TypeScript. If typing test code is annoying, we can use any or type casting in that places.
There was a problem hiding this comment.
I prefer not to mock or mutate modules as possible. So it would be nice to use static imports as default.
We're doing neither here. We're resetting the module which is not yet supported with Jest and ESM. So until then, we need to re-require.
There was a problem hiding this comment.
Thank you, I have two questions.
- Why do we need to reset the module even though we haven't used mock or mutating modules?
- Can't we reset modules with Jest even if we transpile ESM to CJS by
tsc?
| @@ -32,102 +27,102 @@ describe('ChildMapping', () => { | |||
|
|
|||
| it('should support mergeChildMappings for adding keys', () => { | |||
| let prev = { | |||
| one: true, | |||
| two: true, | |||
| one: 'one', | |||
There was a problem hiding this comment.
This looks like runtime behavior changed. But the PR title says it's only about test. Could you clarify why this change here is necessary?
There was a problem hiding this comment.
The type is ReactChild, which doesn't accept boolean, so I've changed the test data to String.
There was a problem hiding this comment.
Sure but that's not a reason to change runtime behavior, no? So far this looks like a breaking change just so that our tests are type-checked?
There was a problem hiding this comment.
What does "runtime behavior" mean? This doesn't change any runtime behavior. The TypeScript migration causes this change. Do you mean that we shouldn't do any incompatible changes with DefinitelyTyped?
This migrates test files to TypeScript. This is not necessary, but it's useful to test the TypeScript definition.