Now that we are using @expo/react-native-action-sheet we need to introduce the following flow types to use the action sheet in our codebase. I read through the source code, and used types.ts when writing the flow types.
Linear Task: ENG-2145
Details
Details
All the testing for this diff came from its use in D5524. In that diff I was able to implement the action sheet using the flow types I created in this diff, and I received no flow errors which I believe to be a good indicator that everything is correct. I noticed in the flow docs, there was a section to write tests; however, I also noticed that the other flow-typed library definitions in our code base also don't have any tests, but if this is something we want to get done, I will take a look into it
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/flow-typed/npm/@expo/react-native-action-sheet_vx.x.x.js | ||
|---|---|---|
| 17–31 ↗ | (On Diff #18036) | This was taken from the flow typed library definitions of bottom-tabs_v6 |
Comment Actions
Can we preface property names with + to mark them as ReadOnly and use $ReadOnlyArray instead of Array if possible.
| native/flow-typed/npm/@expo/react-native-action-sheet_vx.x.x.js | ||
|---|---|---|
| 4–14 ↗ | (On Diff #18036) | We can probably cut this comment |
| 33–43 ↗ | (On Diff #18036) | Shouldn't we be prefacing the property names here with + to mark them as ReadOnly? |
| 34 ↗ | (On Diff #18036) | Can we use $ReadOnlyArray instead of Array? (Here and elsewhere in the file) |
Comment Actions
Looks close, it looks like we're missing a property in ActionSheetOptions (correct me if I'm wrong) and we can make the StyleObj comment a little more informative.
Preemptively adding @ashoat as blocking reviewer because he has a lot more experience with flow.
| native/flow-typed/npm/@expo/react-native-action-sheet_vx.x.x.js | ||
|---|---|---|
| 5–8 ↗ | (On Diff #18046) | Maybe it's just me, but I feel like I don't have the sufficient context for this comment to make sense here. Maybe instead mention what you did in the annotation:
? |
| 37–38 ↗ | (On Diff #18046) | Should we also include +titleTextStyle?: TextStyleProp, ? Looks like it's included in the TypeScript type: https://github.com/expo/react-native-action-sheet/blob/fc9cefa2c15dc54e0c7fa76b7a86c46bf06c67c2/src/types.ts#L30 |
| native/flow-typed/npm/@expo/react-native-action-sheet_vx.x.x.js | ||
|---|---|---|
| 5–8 ↗ | (On Diff #18046) | @atul and I were discussing what the comment on lines 5-8 should be, we were wondering if we should keep this as is or change it to
or use something else entirely. Since you originally wrote this comment in the React Navigation libdef, we would love to hear your thoughts |
| native/flow-typed/npm/@expo/react-native-action-sheet_vx.x.x.js | ||
|---|---|---|
| 5–8 ↗ | (On Diff #18046) | Honestly either way is probably fine. flow-typed has some new mechanism to share types across libdefs... at some point I'll take a look at using that to avoid copy-pasting so much between libdefs |