Add eslint #97
Add eslint #97
Conversation
|
I can split this into two or just take the eslint if the --fix is too much for one review. We also could use something like eslint-nibble to do this one at a time. Although this feels more like ripping off a bandaid. |
There was a problem hiding this comment.
This looks solid overall to me, thanks for doing the big hairy pass! 🏆
I flagged a few that looked suspicious to me that I think might throw errors due to var vs const/let. There are also some types of errors that didn't pop up that I'm surprised about (eg not using loop indices as react keys) but we can debug that later.
It'd be good do build all the packages after these changes and test the local demo page to unsure no egregious errors.
| **/build/* | ||
| **/node_modules/* | ||
| **/coverage/* | ||
| **/_gh-pages/* |
There was a problem hiding this comment.
I don't think this line is necessary since there is no gh-pages branch for this repo.
| @@ -1,7 +1,7 @@ | |||
| export default function center(scale) { | |||
| var offset = scale.bandwidth() / 2; | |||
| let offset = scale.bandwidth() / 2; | |||
There was a problem hiding this comment.
I think this actually still needs to be var because of the closure, did you test the gallery locally to see if there were no errors?
There was a problem hiding this comment.
I did not. :'( I might have had too much trust in the --fix.
That was a dumb mistake. From now on I will always test the gallery locally!
As for the var. Don't lets work in "child" scopes, but not in above scopes?
There was a problem hiding this comment.
Example: https://repl.it/J2dE
| @@ -104,7 +104,7 @@ export default class Gallery extends React.Component { | |||
| <div className="details"> | |||
| <div className="title">Lines</div> | |||
| <div className="description"> | |||
| <pre>{`<Shape.Line />`}</pre> | |||
| <pre>{'<Shape.Line />'}</pre> | |||
There was a problem hiding this comment.
not sure if the backticks were intentional for styling?
| @@ -15,7 +15,7 @@ export function createCircles({ | |||
| stroke, | |||
| strokeWidth, | |||
| strokeDasharray, | |||
| className | |||
| className, | |||
| }) { | |||
| return corners.map(([cornerX, cornerY]) => | |||
There was a problem hiding this comment.
surprised this one didn't complain about no ( + )
| @@ -28,7 +28,7 @@ export default function withParentSize(BaseComponent) { | |||
|
|
|||
| resize(event) { | |||
| if (this.container) { | |||
| var boundingRect = this.container.getBoundingClientRect(); | |||
| const boundingRect = this.container.getBoundingClientRect(); | |||
There was a problem hiding this comment.
this might be another instance where you need a var so it can be used in the closure
There was a problem hiding this comment.
I think you can still use const here since the closure is contained within the same closure that var is defined in.
In general, I think let and const work the same way most variable assignments work in other languages, but var is the nasty one :'(
There was a problem hiding this comment.
I think you're right about the scope, played with it for a while on babel + locally. I remembered having an issue with something similar in a different repo but I think the root cause was something else. thanks for the SE link 👍
|
Awesome thanks @Flaque, I'm going to be away from the computer this weekend, but I will take a closer look and give it a proper review on Monday. No need to split it up. |
|
I went back and made sure everything still ran in the gallery. No problems as far as I can tell. |
|
Awesome, thanks for functionally testing the UI! 👏 |
|
I'll go through and resolve the conflicts tonight. |
|
So this got me thinking more than I expected. The gist is, linting is great for large codebases with a large number of engineers contributing all the time. At this time, vx finds itself mostly driven 4 contributors at the moment so I think we can get by without adding a big new step like this for now. Linting has a time and place and i'm not sure if vx is ready for it at this stage of development. I've been using prettier to format on save for the past few weeks and it's been lovely. But that's mostly because i've lived and breathed the airbnb eslint config for the past 4 years and most of the lint fixes we're seeing are my personal preferences vs the airbnb config. For now I think we're fine without it. I don't want to slow down/trip up new contributors just yet. Before the v1 release we can run prettier on the whole codebase and add in the lint config + build step then and we can tend the garden from v1 on. Would love to hear your thoughts on this. I've been thinking about it a lot since the weekend. |
|
I love this plan. Prettier is wonderful <3 If we're all good, I'll close this PR. |
|
Yeah let's close this for now and we'll bring the discussion back to life closer to |
|
Sounds good! |
|
this sounds like a good plan to me 👍 |
Config from @williaster's brilliant dataui.