cleanup and bump dev dependencies #314
Conversation
There was a problem hiding this comment.
Not sure if you squash commits when you merge, but I did notice a typo in one of the commits:
fix snapshot for tracis
Other than that I had a few questions/comments... But looks good 👍
Was going to suggest running prettier on the entire codebase. Found it distracting to have incremental prettier changes in each PR.
.flowconfig
Outdated
| flow-typed/sketch.js | ||
| flow-typed/sketchapp-json-plugin.js |
There was a problem hiding this comment.
I have no issue with this, but the docs state that everything within the flow-typed directory will automatically be treated as a library definition. We could remove the entire [libs] section if we wanted.
There was a problem hiding this comment.
ah yes, I didn't know (not very familiar with flow)!
| ), | ||
| PropTypes.number, | ||
| ]), | ||
| name: PropTypes.string, | ||
| children: PropTypes.node, | ||
| }; | ||
|
|
||
| // $FlowFixMe |
There was a problem hiding this comment.
Can we leave more information as to why this is necessary?
There was a problem hiding this comment.
I'm starting to see this in a few additional files. Perhaps they can be fixed in a single pass after this PR gets merged.
There was a problem hiding this comment.
flow now requires React.Component to define the props it accepts. So we would need to duplicate the info in the prop-types. I'm not keen to duplicate codes and I don't want to lose the runtime check that prop-types bring. So I ignored it for now. Not sure what we should do.
There was a problem hiding this comment.
Not necessary in this PR, but what about https://github.com/brigand/babel-plugin-flow-react-proptypes?
There was a problem hiding this comment.
I thought about it as well. I read somewhere that it didn't really work but worth a try I guess.
| @@ -29,7 +29,6 @@ const walkTree = (tree: TreeNode, context: Context) => { | |||
|
|
|||
| return node; | |||
| }; | |||
| const treeToNodes = (root: TreeNode, context: Context): yoga.NodeInstance => | |||
| walkTree(root, context); | |||
| const treeToNodes = (root: TreeNode, context: Context): yoga.Yoga$Node => walkTree(root, context); | |||
There was a problem hiding this comment.
Nothing wrong here... I have just been trying to figure out what this syntax is: Yoga$Node. I have seen it elsewhere in the code within React$Component & React$Element
There was a problem hiding this comment.
it's the type definition that comes with yoga: https://github.com/facebook/yoga/blob/master/javascript/sources/entry-common.js#L123
There was a problem hiding this comment.
Ahhh. So it is just a convention. Awesome. Thanks!
__tests__/components/RedBox.js
Outdated
| invariant(false, 'THIS IS AN ERROR'); | ||
| } catch (e) { | ||
| const tree = renderer.create(<RedBox error={e} />).toJSON(); | ||
| it.skip('renders simple errors', () => { |
There was a problem hiding this comment.
Can we mock the error?
Or... split this into 2 tests where each test case is wrapped in a conditional to test the node version and only execute if supported?
There was a problem hiding this comment.
yeah, I'll try to mock. I don't even have the same stack trace locally than on travis...
|
and yes, we always squash the PRs |
sketch-constantsandsketchapp-json-plugineslint-config-sketchinstead of@jongold/eslint-config-sketch(connect Automate generation of eslint globals & flow definitions #313 )