clean up flow errors(#528) #549
Conversation
| } | ||
| Object.keys(users).forEach((userKey) => { | ||
| users[userKey].socket && users[userKey].socket.destroy({ timeout: 500 }) | ||
| }); |
There was a problem hiding this comment.
Changed it to the Object.keys() version only because it fixes the flow error even though using Object.values() is obviously simpler and cleaner.
please refer to :
| @@ -152,7 +152,8 @@ describe('Full walkthrough', function () { | |||
| }) | |||
|
|
|||
| it('Should open sockets for Alice and Bob', async function () { | |||
| for (let user of Object.values(users)) { | |||
| const userList = Object.keys(users).map((userKey) => users[userKey]) | |||
| for (let user of userList) { | |||
There was a problem hiding this comment.
Changed it to the Object.keys() version only because it fixes the flow error even though using Object.values() is obviously simpler and cleaner.
please refer to :
facebook/flow#2221
https://github.com/facebook/flow/blob/master/lib/core.js#L65
| const { contracts } = store.state | ||
| return Object.keys(contracts) | ||
| .filter((key) => contracts[key].type === 'GroupContract') | ||
| .map((key) => ({ groupName: state[key].groupName, contractID: key })) |
There was a problem hiding this comment.
Changed it to the Object.keys() version only because it fixes the flow error even though using Object.entries() is obviously simpler and cleaner.
please refer to :
facebook/flow#5838
https://github.com/facebook/flow/blob/master/lib/core.js#L48
There was a problem hiding this comment.
Very informative comment/explanation, thank you for explaining why this was changed and linking to the appropriate Flow issue.
Can you insert a 1-line comment linking to that flow issue on line 148 explaining that? (So that any developers reading this who are tempted to rewrite it back to .entries understand why .keys is being used?
There was a problem hiding this comment.
Thank you for reviewing and I will do that :)
.Gruntfile.babel.js
Outdated
| @@ -315,7 +315,7 @@ module.exports = (grunt) => { | |||
| break | |||
| case 'BUNDLE_END': | |||
| grunt.verbose.debug(this.nameArgs, event.code) | |||
| const outputName = watchOpts.output.file || watchOpts.output.dir | |||
| const outputName = watchOpts.output.dir; | |||
There was a problem hiding this comment.
This needs to be as it was for a few reasons:
- The semi-colon violates our linter (StandardJS)
- Depending on whether this line points to a file or a directory, either
watchOpts.output.fileorwatchOpts.output.dirwill be defined
| return events.reverse().map(e => b64ToStr(e)) | ||
| if (Array.isArray(events)) { | ||
| return events.reverse().map(e => b64ToStr(e)) | ||
| } |
There was a problem hiding this comment.
Nice, this makes sense, since theoretically there could have been an error and events could be undefined (in fact there could be an exception thrown too, and we don't handle that here...).
| const { contracts } = store.state | ||
| return Object.keys(contracts) | ||
| .filter((key) => contracts[key].type === 'GroupContract') | ||
| .map((key) => ({ groupName: state[key].groupName, contractID: key })) |
There was a problem hiding this comment.
Very informative comment/explanation, thank you for explaining why this was changed and linking to the appropriate Flow issue.
Can you insert a 1-line comment linking to that flow issue on line 148 explaining that? (So that any developers reading this who are tempted to rewrite it back to .entries understand why .keys is being used?
| console.log('transitionAfterEnter') | ||
| // Trigger/Target has opacity: 0 by default, so let's force to stay 1 after the animation finishes. | ||
| // Velocity(el, { opacity: 1 }, { duration: 0 }) | ||
| el.style.opacity = 1 | ||
| el.style.opacity = '1' |
There was a problem hiding this comment.
Is this right? Did Flow say to change this?
There was a problem hiding this comment.
Yes, this is what flow spits out about that code and it seems to be referring to this type defintion.
A possible solution that is suggested in this issue actually fixes the error but I'm afraid it could make the code verbose just to keep flow happy.
(changing it to
const elStyle: Object = el.style
elStyle.opacity = 1
)
There was a problem hiding this comment.
Hmm. OK, makes sense. I guess as long as what you have here works the same it's fine! 👍
| // Use only opacity to fadeIn/Out because it's faster than | ||
| // Velocity's 'fadeIn' feature (it doesn't use display) | ||
| // Velocity(el, { opacity: 0 }, { duration: 0 }) | ||
| el.style.opacity = 0 | ||
| el.style.opacity = '0' |
Cleaning up the remaining flow errors
** Left the error about Gruntfile.babel.js:228:27( An error caused by flow not recognizing grunt.log('....'.bold) ) unsolved because it seems to happen only because flow can't recognize that special usage of grunt.log().