Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eslint + Prettier = 😍 #19

Merged
merged 16 commits into from
Mar 25, 2017
Merged

Eslint + Prettier = 😍 #19

merged 16 commits into from
Mar 25, 2017

Conversation

grabbou
Copy link
Member

@grabbou grabbou commented Mar 25, 2017

Fixes #17

In this PR:

  1. I've fixed remaining Eslint issues
  2. Adjusted config a bit (e.g I disabled dynamic require eslint line as Flow already warns about that, having to ignore two rules at one is a bit cumbersome)
  3. Added missing dependencies (they worked because React Native loads them and Yarn makes a nice flat dependency tree)
  4. Removed polyfill of global - I added it at the very beginning at the point I was hacking around trying to get things working. InitializeCore already does that
  5. Fixed prettier glob pattern in npm scripts - it was expanded by shell, not by Prettier (missing "). That caused it to not format nested files.
  6. Added eslint-config-prettier to turn off settings that either interfere with Prettier (--fix option) or warn about things prettier changes for us anyway (like trailing commas)
  7. Setup precommit script with Husky to run every time we commit - it automatically fixes small eslint issues for you, runs prettier and adds files to existing commit
  8. Moved few files into package.json to reduce dot files (I hate them)

@@ -53,7 +50,6 @@ function createServer(

// Middlewares
appHandler
.use(morgan('tiny'))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satya164 I think for that to work, we should probably expose a flag verbose on a command line tools in the future and later, use something more comprehensive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this when we actually have it. Otherwise it's impossible to see if packager actually received the request. Though I think printing requests is pretty basic and shouldn't depend on --verbose

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printing all requests like the above does should depend on verbose flag as informing about devtools ui request is not something average user would be interested in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I won't be interested in requests for devtools UI, but I'll certainly be interested in requests for the bundle and assets.

@grabbou grabbou changed the title Fix eslint and formatting Eslint fixes Mar 25, 2017
@grabbou grabbou changed the title Eslint fixes Eslint + Prettier = 😍 Mar 25, 2017
@grabbou
Copy link
Member Author

grabbou commented Mar 25, 2017

Note: We removed dependency on pre-commit. Not sure if on yarn install it will remove the ones that were removed. If not, it means pre-commit won't call its uninstall script. Do it manually, otherwise Husky won't run.

@grabbou
Copy link
Member Author

grabbou commented Mar 25, 2017

CC this issue to @mjackson when we open source that as he wanted me to do the same for react-router

package.json Outdated
"lint": "eslint src",
"prettier": "prettier --single-quote --write src/**/*.js"
"precommit": "lint-staged",
"test": "flow && npm run lint",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think flow should be a separate script. what if I just want to run flow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems valid, will add

@@ -53,7 +50,6 @@ function createServer(

// Middlewares
appHandler
.use(morgan('tiny'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this when we actually have it. Otherwise it's impossible to see if packager actually received the request. Though I think printing requests is pretty basic and shouldn't depend on --verbose

Place it after devTools middleware so that we dont log too much to end users, but request for assets
only.

Also, change url to path as we dont support params (like Packager) and printing them would make that confusing for people.
@grabbou
Copy link
Member Author

grabbou commented Mar 25, 2017

screen shot 2017-03-25 at 20 05 48

@grabbou grabbou removed the request for review from zamotany March 25, 2017 19:07
@grabbou grabbou merged commit a4b1456 into master Mar 25, 2017
@satya164 satya164 deleted the feat/fix-linting branch March 26, 2017 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants