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

Should the gatsby-default-started have a ESlint + Prettier config by default? #11778

Closed
frontendwizard opened this issue Feb 14, 2019 · 33 comments
Labels
good first issue Issue that doesn't require previous experience with Gatsby type: question or discussion Issue discussing or asking a question about Gatsby

Comments

@frontendwizard
Copy link
Contributor

I'm starting out with GatsbyJS and the default started is pretty great, but I find myself having to setup the eslint everytime I start a new gatsby project. Should gatsby-default-started have a default config with the react and prettier plugins by default, since it is strongly recommended for everyone?
I do understand that the default started should be a no brainer and no opinion started for everyone and regarding the linter setup you do have some options, like airbnb vs standard style. Should I go and create a separate started with my own setup and start using?

@gatsbot gatsbot bot added the type: question or discussion Issue discussing or asking a question about Gatsby label Feb 14, 2019
@josefaidt
Copy link
Contributor

IIRC Gatsby ships with a small (internal) ESLint file, and I thought it used to come with a Prettier dotfile. I may be wrong, though. I wrote the doc for using ESLint with Gatsby but I think I may need to revisit it at this point.

I agree, including those dotfiles in the default starter would be beneficial.

@KyleAMathews
Copy link
Contributor

Gatsby ships an eslint config internally that uses the same ruleset as CRA.

For Prettier, we used to have it but it was removed recently for reasons unclear to me. Perhaps we should add it back?

@DSchau
Copy link
Contributor

DSchau commented Feb 14, 2019

👋

Yup - my doing on this one. See #11285 for more info on that.

So, two things here:

  1. eslint should probably stay removed (it was only in the blog starter, and for little benefit)
  2. Adding back prettier probably makes sense

On 2. we removed it to simplify the starters and because generally it's very easy to add it back yourself. That being said--I do see the value of baking in default packages here, so happy to have another PR addressing the removal and adding prettier back to the starters!

@josefaidt
Copy link
Contributor

@DSchau I may hop on this tonight. Should the prettier config from gatsby's project root be used for this?

@DSchau
Copy link
Contributor

DSchau commented Feb 14, 2019

@josefaidt yeah - that seems reasonable. May want to pull back some of the more opinionated ones (e.g. semi, trailingComma, etc.), but otherwise I think that's a fair starting point.

@frontendwizard
Copy link
Contributor Author

I'd love to contribute on this one too if that's ok, it sounds like a good first issue for those who did not contributed to the codebase yet. :)

@DSchau DSchau added the good first issue Issue that doesn't require previous experience with Gatsby label Feb 14, 2019
@DSchau
Copy link
Contributor

DSchau commented Feb 14, 2019

That'd be great! Whoever does take it just make sure to make a comment here when you start so that we don't double up work here.

Thanks!

@frontendwizard
Copy link
Contributor Author

Sure. I'll wait for @josefaidt 's input on it cause he has the priority since he claimed it first. If he is ok with me taking it, I'll gladly do it. 😃

@josefaidt
Copy link
Contributor

@thefrontendwizard by all means! I will work on updating the ESLint doc. Just keep the name .prettierrc and I'll be good - I'll be referencing that in the doc

@josefaidt
Copy link
Contributor

to add on to the comment regarding the opinionated Prettier rules, I thought we could provide all the default rules. That way they're front and center to edit, as well as not show any bias

@frontendwizard
Copy link
Contributor Author

Ok. I'm filling in the .prettierrc with the defaults mentioned in the docs.

@frontendwizard
Copy link
Contributor Author

I do feel like we could start with the same config made by the project, even with opinionated ones, as long as we point that out on the docs. Specially the trailingComma: "es5", which seems to be the most used option lately.

@frontendwizard
Copy link
Contributor Author

frontendwizard commented Feb 15, 2019

One point for this is the style already adopted in the few lines of code both in the default and on the blog starters.

@josefaidt
Copy link
Contributor

That is a good point, I'll update the eslint doc to reflect that

@frontendwizard
Copy link
Contributor Author

frontendwizard commented Feb 15, 2019

Ok. We can say something like: "Hey, this is how we like our codebase to be. If that's not your jam, tweak it out on .prettierrc". It's easier than ask it to create one and then point it out to prettier docs. Prettier by itself is already opinionated, so there's no point trying not to be. The good thing is that prettier does not get in your way, it does the boring part for you.
That said, this still requires some setup on IDE to actually make prettier work. Shall we add something like lint-staged to get prettier to format the files staged for the commit or that would be too much?

@wardpeet
Copy link
Contributor

I'm a big fan of lint-staged. I think it might make the setup a bit more difficult from a beginners point of view. Like yikes, I can't commit! 😠 (not all IDEs tell you why you can't commit).

@frontendwizard
Copy link
Contributor Author

yup, that makes sense. It could be in the docs though, as a recommendation.

@josefaidt
Copy link
Contributor

There's a doc that goes over setting up Prettier + VSCode

I think it would be beneficial to add another script to the default starter that runs Prettier, then in the ESLint doc I can reference that script saying "modify this script to use ESLint..." or something.

@frontendwizard
Copy link
Contributor Author

something on the lines of

prettify: "prettier --write src/**/*.{js,jsx}",
lint: "eslint src/**/*.{js,jsx}"

?

@josefaidt
Copy link
Contributor

josefaidt commented Feb 15, 2019

Yes just use the prettier script, though. The ESLint doc integrates Prettier with ESLint so the user will be modifying that script.

Something like this?

  • default starter: "prettify": "prettier --write src/**/*.{js,jsx}"
  • ESLint doc:
"scripts": {
  "lint": "eslint src",
  "lint:fix": "eslint --fix src"
}

Do yall think the ESLint doc should have instructions for integrating with VSCode? Since there's a doc for Prettier about it and ESLint will replace Prettier...

@frontendwizard
Copy link
Contributor Author

ESLint will replace Prettier? They are not mutually exclusive. Prettier deals with style and only that. The ideal setup would be to use both ESLint and Prettier.

@josefaidt
Copy link
Contributor

That's my bad not replace Prettier, integrate with it. It'll use Prettier for the formatting

@frontendwizard
Copy link
Contributor Author

I'll add the prettify script on my PR.

@frontendwizard
Copy link
Contributor Author

As for the instructions to integrate ESLint with VSCode, I believe that we should indeed have the instructions on the docs.

@josefaidt
Copy link
Contributor

Cool, I'll update it once I get home this evening

@wardpeet
Copy link
Contributor

so cool 😎 that you guys are collaborating so well! ❤️

I don't have an immediate opinion on what to call the script ^^

@DSchau
Copy link
Contributor

DSchau commented Feb 15, 2019

It was previously called format so I'd recommend that again :)

And echo'ing @wardpeet's comments above. Nice work thus far everyone!

@josefaidt
Copy link
Contributor

I like format a lot!

@frontendwizard
Copy link
Contributor Author

Ook. I'll name it format and add prettier on devDependencies.

@frontendwizard
Copy link
Contributor Author

Just pushed an update on #11786 with the format script and prettier as a dev dependency.

@josefaidt
Copy link
Contributor

Cool, I'll be getting my updates in this evening :)

@josefaidt
Copy link
Contributor

josefaidt commented Feb 16, 2019

Update to #11783 with scripts and VSCode integration

@frontendwizard
Copy link
Contributor Author

I'll close this one due because #11786 solves this by adding a prettier config into all the starters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issue that doesn't require previous experience with Gatsby type: question or discussion Issue discussing or asking a question about Gatsby
Projects
None yet
Development

No branches or pull requests

5 participants