kriasoft / react-firebase-starter

Boilerplate (seed) project for creating web apps with React.js, GraphQL.js and Relay
https://firebase.reactstarter.com
MIT License
4.51k stars 751 forks source link

Fix ESLint warnings #192

Open koistya opened 7 years ago

koistya commented 7 years ago

Is anyone willing to help? Run yarn lint in a terminal window. See package.json/eslintConfig.

image

rajpatel507 commented 7 years ago

I can help if you can provide more info.

koistya commented 7 years ago

@rajpatel507 assuming you have Node.js v6+ and Yarn installed, when you run yarn lint it prints lots of ESLint warnings. It happened after the latest update of eslint and eslint-config-airbnb modules. Some rules can be disabled/tweaked if needed (see package.json/eslintConfig), but ideally we want to reuse AirBnb's ESLint preset as much as possible.

https://github.com/airbnb/javascript https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb

ericlau-solid commented 7 years ago

@rajpatel507 you got what you needed and making progress? Checking in because I am interested. Thanks.

rajpatel507 commented 7 years ago

@ericlau-solid I am getting around 40 errors and 2 warnings, is it normal or i am missing something?

ericlau-solid commented 7 years ago

@rajpatel507 Fixing the errors and warnings is the ask. You know you are done when the package still works and you get no errors and warnings.

rajpatel507 commented 7 years ago

@ericlau-solid some errors indicating that package is not in package.json dependency and still it is used but we have all dependency there in package.json, this is usally when eslint is not finding package.json file in source .I need to investigate why .eslintrc file is not pointing to package.json

ericlau-solid commented 7 years ago

@rajpatel507 there are multiple package.json files. when running eslint, it only looks at the package.json closest to the file in question. All package.json in sub-folders need to have dependencies object correctly set up.

giantrobotbee commented 7 years ago

Is there any status on this?

rajpatel507 commented 7 years ago

@drmanitoba not able to figuring out why error is there even if package is there in dev dependency of package.json

frenzzy commented 7 years ago

@rajpatel507 just add "import/no-extraneous-dependencies": "off" to package.json until the related issue exists (PR)

rajpatel507 commented 7 years ago

@frenzzy Thanks for suggestion, I have added "import/no-extraneous-dependencies": "off" and "env": { "browser": true } in package.json now warning comes down to 17 from 42 earlier, remaining warnings are from missing comma somewhere and other cosmetics.

@koistya should I remove all other warning or create pull request for this changes only?

frenzzy commented 7 years ago

This issue about fixing all eslint errors and warnings

rajpatel507 commented 7 years ago

I changed code according to ESLint rule and some places i have disabled ESLint for a line. it solved all error message from ESLint.

Still i am getting these two warning can anybody help me how to solve these. image

frenzzy commented 7 years ago

I think you can just add inline comment to make it clear for developers that this was done deliberately and not by mistake, for example:

<div
  // eslint-disable-next-line react/no-danger
  dangerouslySetInnerHTML={{ __html: html }}
/>

ref react/no-danger

rajpatel507 commented 7 years ago

I have created pull request for same.

frenzzy commented 7 years ago

@rajpatel507 Awesome! I just reviewed it #199