Strictness migration #3210
Strictness migration #3210
Conversation
ds300
force-pushed
the
strictness-migration
branch
4 times, most recently
from
9e2959a to
c8b01d4
Compare
| const _ = require("lodash") | ||
| const path = require("path") | ||
|
|
||
| const HERO_HELPER_FILENAME = ".i-am-helping-out-with-the-strictness-migration" |
| command: | | ||
| set -e | ||
| current_issues=$(node scripts/strictness-migration.js count) | ||
| git checkout origin/master |
There was a problem hiding this comment.
My initial thought was this would go nicely in a Danger rule, but this actually makes way more sense as its own build step, since we don't need to worry about changing the git head back or anything 👍 The only benefit a Danger rule would have is the ability to report back to the PR in a comment. To do it, I think we'd need to run Danger in its own job like this anyway.
| @@ -28,12 +29,12 @@ export const FilterModalNavigator: React.SFC<FilterModalProps> = props => { | |||
|
|
|||
| const handleClosingModal = () => { | |||
| dispatch({ type: "resetFilters" }) | |||
| closeModal() | |||
| closeModal?.() | |||
There was a problem hiding this comment.
Weird that we need the ., but that might be my Swift bias. In Swift, we'd write this as closeModal?()
| @@ -15,7 +15,7 @@ | |||
| "plugins": [{ "name": "typescript-styled-plugin" }], | |||
| "pretty": true, | |||
| "resolveJsonModule": true, | |||
| "strict": false, | |||
| "strict": true, | |||
|
@ashfurrow if this is 🍏for you then I'll fix the conflicts and merge tomorrow morning UK time, then socialise the info in the PR description in #front-end-ios. Sound good? |
|
@ds300 Yeah, looks good to me! Merging in the morning UK time works, to avoid merge conflicts, and then we'll have the FE iOS Practice meeting in the morning NYC time, to catch people up 👍
|
ds300
force-pushed
the
strictness-migration
branch
from
c8b01d4 to
164a2aa
Compare
The approach taken here is as was discussed in slack
i.e.
strictmode on intsconfig.jsonSTRICTNESS_MIGRATIONin commentsSTRICTNESS_MIGRATIONfrom appearingSTRICTNESS_MIGRATIONissuesDX Changes
We are temporarily requiring PRs to be up-to-date before merging, because if people forget to update their branches it will cause type errors in master.
Life will be harder at first. Strict mode definitely has a learning curve, but once you get up to speed it will barely be noticeable, and will be a big win for preventing run time bugs and increasing maintainability.
The biggest source of pain will probably be dealing with Metaphysics data.
MP was not written with strictness in mind so there are lots of places where fields that shouldn't be nullable are in fact nullable. The good news is that it's not a breaking change to make nullable fields non-nullable, and it should be pretty straightforward to do.
If you want to be extra helpful in refactoring older code to be
strictyou can runand then an extra check will be added to your pre-commit hook. This check will fail if any of your staged files have
STRICTNESS_MIGRATIONtags inside, so you are encouraged to spend a few minutes cleaning them up.Note that this is opt-in. If you want to be heads-down on shipping product rather than improving old code, that takes priority.
If you add any
STRICTNESS_MIGRATIONtags, e.g. by copy-pasting code, CI will fail.