Fix flow bugs #91
Fix flow bugs #91
Conversation
|
Note: This PR doesn't solve all the errors that upgraded the packages discovered for us. Notably, I left the following as warnings since they are clearly errors but they require more thought so they should probably be done in a separate PR:
|
| @@ -1,5 +1,12 @@ | |||
| { | |||
| "parser": "babel-eslint", | |||
| "parserOptions": { | |||
| "ecmaVersion": 6, | |||
There was a problem hiding this comment.
This is because the version of eslint we use is super recent so it can support a higher version of babel than we support for our actual codebase. Notably, it includes some change that requires you to change the order decorators are associated in your class. To disable this feature on linting (to match the version we are actually compiling with) I had to add this very cryptic extra field.
You can see the occasional Github issue come up related to this topic: babel/babel-eslint#662
| @@ -112,8 +113,8 @@ export async function saveAsAdaAddresses( | |||
| addresses: Array<string>, | |||
| addressType: AddressType | |||
| ): Promise<void> { | |||
| const mappedAddresses: Array<AdaAddress> = addresses.map((hash, index) => | |||
| const mappedAddresses: Array<AdaAddress> = addresses.map((hash, index) => ( | |||
There was a problem hiding this comment.
I’m curious. I think it shouldn’t be necessary to add those 2 extra ( ). is it a configuration of the eslint?
There was a problem hiding this comment.
The rule in Javascript is you can have a => on a new line without putting any parenthesis.
The eslint rules says if you put a => on a separate line, you should put () around it even if it's not required.
I think this makes sense. It's the same reason linters for various languages complain when you have an if that don't use curly braces. It's a common source of bugs when refactoring code.
There was a problem hiding this comment.
In general though eslint is super strict that parenthesis should go on new lines. I agree that in cases like this it improves readability but in other places in the PR it you can see it just kind of bloats the code. Usually in cases where there is upside & downside to a linter rule I tend to just follow what the linter says since at least it allows us to standardize on some rule even if we don't always agree with it.
There was a problem hiding this comment.
eslint is a rule applier so it depends on the rules that we want. I didn’t know if you added the rule / if you preferred that way or both. 👍
|
@SebastienGllmt this is awesome!!! I would like to ask you if we could sync with the prettier formatter 🙏🙏🙏 so we don’t need to spend time doing crazy formating |
SebastienGllmt
added
NFC
As I was documenting some stuff, I noticed there were some flow mistakes in our code.
@sarveshwar-singh sorry for the merge conflict 🙏
The ugly
// eslint-disable-next-line space-infix-opsis because of a bug ineslintthat seems to have been fixed in 2016 but we have to upgrade our package to use it (hence why the branch name of this PR seems to be unrelated)