Convert WelcomeScreen to hooks and Typescript #943
Conversation
|
Mocking those buttons and everything else was pretty simple. If you mock a component as a string, it serializes it perfectly :) |
| import styles from './styles'; | ||
| import { ThunkDispatch } from 'redux-thunk'; | ||
|
|
||
| const WelcomeScreen = ({ |
| next, | ||
| }: { | ||
| dispatch: ThunkDispatch<any, any, any>; // TODO: replace any | ||
| next: (params: { signin: boolean }) => {}; |
There was a problem hiding this comment.
That's not what it currently is
https://github.com/CruGlobal/missionhub-react-native/pull/943/files/8c347ed91cc7ff3a26b32627afd1a1238c86a295#diff-d9ce3bef7d67219ec0212cf2e853a896R34
It should probably be camelCased but
signin is correct for now. I could rename it here or in another PR...
|
Well I can't search in my IDE for a class in an index file... is there a plugin to be able to do that with TypeScript? |
|
I've started using VSCode the last month or so... Man indexing takes a while in IntelliJ if you haven't used it in a while. I hit Should be part of JavaScript Support: VSCode will let you hover and peek at definitions: Not quite sure what you're asking... |
OzzieOrca
force-pushed
the
welcome-screen-hooks-typescript
branch
from
8c347ed
to
96fc165
Compare
OzzieOrca
force-pushed
the
welcome-screen-hooks-typescript
branch
2 times, most recently
from
2ad77ec
to
5771baf
Compare
OzzieOrca
force-pushed
the
typescript
branch
2 times, most recently
from
fddfdc8
to
ea0c326
Compare
OzzieOrca
force-pushed
the
welcome-screen-hooks-typescript
branch
from
5771baf
to
c29e9e0
Compare
OzzieOrca
force-pushed
the
welcome-screen-hooks-typescript
branch
from
c0ef85b
to
9ff3e1b
Compare
- Add new testing helpers
OzzieOrca
force-pushed
the
welcome-screen-hooks-typescript
branch
from
9ff3e1b
to
7909e63
Compare
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| dispatch: ThunkDispatch<any, null, never>; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| next: (params: { signin: boolean }) => ThunkAction<void, any, null, never>; |
There was a problem hiding this comment.
Should dispatch and next be defined as Interfaces? They will be used across many screens.
There was a problem hiding this comment.
I think anything thunk related is going to be really hard to type right. Especially as we're transitioning. You're supposed to specify the type of the last action dispatched and what the return value is. But usually we just dispatch more thunk actions...
Ya if these end up looking the same we should make interfaces. But if we do get some of redux typed we might be able to give each one specific action types and stuff.
|
|
||
| const navigateToNext = (signin = false) => { | ||
| // Remove the back handler when moving forward | ||
| disableBack.remove(); |
There was a problem hiding this comment.
Won't this already happen on unmount because of the return statement in useEffect?
There was a problem hiding this comment.
This was here before. But I just tested it and the screen doesn't get unmounted until the navigate reset at the end of onboarding.
There was a problem hiding this comment.
From what I can remember about this, this screen doesn't unmount when you move forward so we had to remove this so it wouldn't affect the other pages ahead of it.
There was a problem hiding this comment.
Ya... My assumption from testing it is stack navigators don't unmount components still in the stack. Resetting the stack does unmount it but that doesn't happen until after the celebration screen at the end of onboarding.
| navigateToNext(true); | ||
| }; | ||
|
|
||
| const allowSignIn = useNavigationParam('allowSignIn'); |
There was a problem hiding this comment.
Will this be the new standard instead of using mapStateToProps?
There was a problem hiding this comment.
I think it's a whole lot nicer and clearer where the variable came from. I guess if you need to actually use it in mapStateToProps, you'll still have to pull it off the navigation props.
There was a problem hiding this comment.
Right, there might be some operations using nav props that we still need mapStateToProps for, but for the most part it is nice to avoid that and use the nav props with the hook. Also I imagine that some of our mapStateToProps work could eventually be moved into the functional component using hooks, we'll see.
There was a problem hiding this comment.
Ya :) Unfortunately I can't upgrade react-redux until Enzyme supports Context or we migrate away from Enzyme...
| <WelcomeScreen next={next} />, | ||
| allowSignInVariantConfig, | ||
| ); | ||
| fireEvent(getByTestId('sign-in'), 'onPress'); |
There was a problem hiding this comment.
Interesting. This might simplify a lot of the testing set-up. I gotta get familiar with react-native-testing-library as well!
There was a problem hiding this comment.
Ya let's have a conversation about testing sometime :) I like the new helper function and expanded upon it and fixed a few things in my graphql branch. I think react-native-testing-library is a good compromise of old and new. We can mock stuff to approximate shallow rendering and shallow snapshots. But it tries to keep you from calling implementation details directly.
I've also looked at native-testing-library but it's a lot more opinionated. It pushes you towards integration testing and doesn't support events on mocked components. testing-library/native-testing-library#17
Enzyme just still doesn't support the new React Context APIs used by a lot of the newer libraries or Hooks...
Welcome to the future 馃殌 Maybe... 馃槂 I'm looking for feedback. Feel free to question anything. I stole some of this out of #891.
For context, I shared this article about deep rendering but mocking complex child components with Daniel today. @dsgoers I haven't mocked anything yet but give me your thoughts on what we should do. I kinda like seeing the built in
<View>and<Text>components all the way down but the styling is a little overwhelming. Maybe I should start by mocking<Button>if it's got too much going on. Not really sure what to do with<Flex>.<Text>might be simple enough just to render all the way down but then again the styling adds noise to the snapshot.I can work on a helper function tomorrow to maybe mock and serialize the params of a mocked component somehow it that seems like something worth trying.