breaking: Migrate codebase to typescript #101
Conversation
Codecov Report
@@ Coverage Diff @@
## main #101 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 149 151 +2
Branches 45 47 +2
=========================================
+ Hits 149 151 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
@Ne3l looks very promising 🚀
I've done first pass of the review and added some TS suggestions for improving typing.
Additionally you probably should use // @ts-expect-error instead of // @ts-ignore when we know that we are hacking TS.
|
@mdjastrzebski Thanks for reviewing, i'm on holidays now but will take a look at the MR at the beginning of september |
Ne3l
force-pushed
the
typescriptMigration
branch
from
7551917 to
ffa6b59
Compare
|
@mdjastrzebski I think i answer/fix all the open threads, please take a look. |
Ne3l
force-pushed
the
typescriptMigration
branch
from
e8e6c9b to
1c20f5c
Compare
|
@mdjastrzebski Not sure what's the etiquette on this repo, but can you resolve the comments that are fixed and take a look at the rest? i've rebased the changes of #110. |
|
@Ne3l I've closed comments that I think are resolved. Sometimes it's hard to easily figure out if the given comment is solved or not, as they disappear from "Files changed" view. |
Ne3l
force-pushed
the
typescriptMigration
branch
from
9c6ff77 to
a105a87
Compare
|
Hi @mdjastrzebski it's me again 😅 i have fixed most of your comments except the narrow one as i'm not sure i understand what do you mean. Lets hope this is the good and we can merge this typescript setup as rebasing each new commit is a bit of a pain in the ass. |
|
@Ne3l I've reviewed the changes and made some minor tweaks to adjust typing or restore original logic where appropriate. Pls take a look at the changes if everything looks good, we can plan merging it tomorrow. I am wondering whether to release this as a new major version/breaking change. The reasoning behind it is that we tweaked some external types and this might cause user tests to fail. On the other hand the logic of all of the matchers should stay the same, so minor version bump could also do. |
mdjastrzebski
changed the title
| result = result.concat(collectChildrenText(child)); | ||
| } | ||
| const result: string[] = []; | ||
| element.children.forEach((child) => { |
There was a problem hiding this comment.
out of curiosity, why the forEach instead of the for of?
There was a problem hiding this comment.
No particular reason beside subjectively slightly easier to read.
|
|
||
| type Style = TextStyle | ViewStyle | ImageStyle; | ||
| type StyleLike = Record<string, unknown>; |
There was a problem hiding this comment.
why the styleLike? passing an undefined as a styleProp is valid but the flatten won't return a Record
There was a problem hiding this comment.
Thanks for pointing out potential undefined return from StyleSheet.flatten() I've added check for that.
Regarding why Record and not concrete Style, it has to do with Object.keys() not handling well Style, so we either need to we sprinkle Object.keys or StyleSheet.flatten with as typecasts. Since flattened style is essential Record<string, any> or undefined, then it makes sense to use Record as it does not quire additional typecasts in the checking functions downstream.
There was a problem hiding this comment.
Just FYI, that StyleSheet.flatten may be deprecated in some far future: https://github.com/react-native-community/discussions-and-proposals/pull/496/files#diff-8e74bede0f5d8ed8c232710e7e2fef8af85fdb5fc65c23bcb415e4e6beafb518R310
There was a problem hiding this comment.
Thanks for heads up, I think we good for now and we'll have to adjust JS implementation when something new comes to replace flatten().
| @@ -6,15 +6,26 @@ exports[`.toHaveStyle handles negative test cases 1`] = ` | |||
| - Expected | |||
|
|
|||
| backgroundColor: blue; | |||
| - transform: [{"scale": 1}]; | |||
| + transform: [{"scale": 2}];" | |||
| transform: [ | |||
There was a problem hiding this comment.
i had the doubt when doing the change, do we want to output everything or just the result which is different than the expectation?
There was a problem hiding this comment.
The idea behind this is that particular style attributes are not divisible, they need to match as units.
Normally this is not an issue as most style attributes are string, number, etc. The only two Array attributes are transform and fontVariant and we want to output the whole array in this case.
|
LGTM regarding the imports i saw you done some ordering. There is an eslint plugin for it that you may want to use https://eslint.org/docs/latest/rules/sort-imports |
|
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
closes #81.
Why:
Typescript provide type safety and ease of use for developers.
How:
As the repository is quite small, migrated all in once.
Use typescript compiler instead of babel to create the dist build. (Babel with react-native-metro preset is optimised to run JS on hermes/jsc not node but it can be configured for it)
Still some parts of the code are not 100% solved but i believe this is a good base to iterate on.
Checklist:
@ts-ignorecommentsThis is a BREAKING CHANGE