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.

Sign up for GitHub

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

Jump to bottom

Standardise code style within the organisation #65

Closed
grabbou opened this issue Dec 7, 2018 · 26 comments
Closed

Standardise code style within the organisation #65

grabbou opened this issue Dec 7, 2018 · 26 comments
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject

Comments

@grabbou
Copy link
Member

grabbou commented Dec 7, 2018 •
edited
Loading

I am about to set up "Prettier" in my project along "ESLint" to keep it consistent across some open-source work that I do and easier for contributors to work without worrying about internal patterns.

I've noticed that we don't have a general "community" preset. There is "eslint-config-fb" that is used by React Native. Unfortunately, it's a part of a project that is not meant to be consumed by 3rd parties.

We also do have "eslint-config-callstack", which is the one that I was planning to use for the CLI. It has Prettier integrated as a ESLint rule and makes it really easy to fix all the issues within your codebase by just running eslint --fix.

However, I am afraid of several inconsistencies that this may cause across community project. For instance, there are projects that don't use Prettier or ESLint at all. There might be some that use a different preset.

Do we care enough to enforce a "standard" ESLint/Prettier config as a part of RNCommunity to make it consistent to contribute? Say, we have "react-native-community/eslint-config-react-native", which also becomes a "recommended" preset for React Native?

Or we don't care and every project is allowed to go on its own?

PS. If we do care, I am happy to do it. We can fork our "callstack" preset as it's already set up for being consumed from "npm" and just adjust the rules.
@kelset kelset added the 🗣 Discussion This label identifies an ongoing discussion on a subject label Dec 7, 2018
@kelset
Copy link
Member

kelset commented Dec 7, 2018

Uhm can't we use simply enforce the one FB uses for the main repo?

@TheSavior what do you think?

@grabbou
Copy link
Member Author

grabbou commented Dec 7, 2018 •
edited
Loading

eslint-config-fb is part of this repository: https://github.com/facebook/fbjs that clearly states:

Note: If you are consuming the code here and you are not also a Facebook project, be prepared for a bad time. APIs may appear or disappear and we may not follow semver strictly, though we will do our best to. This library is being published with our use cases in mind and is not necessarily meant to be consumed by the broader public. In order for us to move fast and ship projects like React and Relay, we've made the decision to not support everybody. We probably won't take your feature requests unless they align with our needs. There will be overlap in functionality here and in other open source projects.

Don't want to run into problems if someone internally changes the configuration. I think that having our own would be a bit better as far as decoupling.

@elicwhite
Copy link

I don’t think that is the one we use for React Native. We use the eslintrc that is in the root of the repo. In fact all js code on our mobile monorepo at FB extends from the React Native config.

@grabbou
Copy link
Member Author

grabbou commented Dec 7, 2018 via email

@mmazzarolo
Copy link
Member

I would suggest using the default prettier preset + the create-react-app eslint config (using eslint-config-react-app or eslint-plugin-react-app) but I'm not sure if enforcing them (or any other ruleset, like a TSLint config) would be a good approach.

@cpojer
Copy link
Member

cpojer commented Jan 16, 2019

I strongly believe the community org should directly follow the React Native main repo. Having a common set of standards shared with the main repo will make it less confusing for everyone and easier to collaborate, as well as move code from one repository to others. The react-native-cli repo for example uses bracket spacing and doesn't use trailing commas. I think it would be best to invert those settings and align with RN.

@grabbou
Copy link
Member Author

grabbou commented Jan 16, 2019

In order to do it, we would need to create a preset somewhere else than in "fbjs" repository to avoid consuming private projects.

See this comment for details: #65 (comment)

Since this may require Facebook to invest some time into making it an independent, open-source, project, I think we can create a "react-native-community" preset, e.g. react-native-community/eslint-config-react-native-community, that "extends" Facebook one.

@cpojer
Copy link
Member

cpojer commented Jan 16, 2019

To start, I would recommend unifying the prettier config, and worrying about the eslint stuff later.

@grabbou
Copy link
Member Author

grabbou commented Jan 21, 2019

Do you want me to create a new repository then?

@cpojer
Copy link
Member

cpojer commented Jan 22, 2019

Yeah, go for it. I guess if you can publish a react-native-community eslint + prettier config based on RN that all downstream projects use, that would be ideal. Even better if we could adopt that in RN too.

@elicwhite
Copy link

Once the core repo has a monorepo organization and can host multiple packages we should have the config package there. All JS in FB’s mobile monorepo extends from React Native’s eslint config so moving it to a separate repo might cause some additional pain for FB.

@cpojer
Copy link
Member

cpojer commented Jan 22, 2019

I'm fine with short term duplication. I care more about making sure react-native-community follows the react-native repo for now.

@michalchudziak
Copy link

I can migrate eslint-config-fbjs to react-native-community/eslint-config-react-native-community if you still need an additional pair of hands to help.

@grabbou
Copy link
Member Author

grabbou commented Feb 5, 2019 via email

@elicwhite
Copy link

I don't think we want eslint-config-fbjs. I think we want this file from the react-native repo: https://github.com/facebook/react-native/blob/master/.eslintrc

@cpojer
Copy link
Member

cpojer commented Feb 5, 2019 via email

@grabbou
Copy link
Member Author

grabbou commented Feb 5, 2019 via email

@cpojer
Copy link
Member

cpojer commented Feb 5, 2019 via email

@kelset
Copy link
Member

kelset commented Feb 5, 2019

Ideally we’ll keep it in the RN repo and reuse it in the community repos.

single source of truth FTW 👍

@michalchudziak
Copy link

@TheSavior thanks for the heads up!

@cpojer What's the advantage of keeping it in react-native repo? I feel like it would be cleaner to keep in separately in the community, it's just an opinion tho.

@cpojer
Copy link
Member

cpojer commented Feb 6, 2019

@michalchudziak React Native will use the same settings and it's the source of truth. We use those internally at FB as well. Basically I want to extract what we use in RN into a separate package that is then used within RN and in the community packages.

@SimenB
Copy link

SimenB commented Feb 6, 2019

Sorta, kinda, in a weird way, related: could that hypothetical package take over for eslint-config-fb-strict which lives in the Jest monorepo? https://github.com/facebook/jest/tree/master/packages/eslint-config-fb-strict

@cpojer
Copy link
Member

cpojer commented Feb 6, 2019

I think that package is stricter than what React Native uses, no?

@SimenB
Copy link

SimenB commented Feb 6, 2019

IDK, I haven't looked at the rules. But makes sense to me to converge on some strict config and not have yet another version? Maybe not

@dratwas dratwas mentioned this issue Feb 28, 2019
Closed
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Mar 1, 2019
@dratwas @facebook-github-bot
Move eslint/prettier config to packages/eslint-config-react-native (#…
e903d80 
…23688)

Summary:
Basing on discussion from this issue - react-native-community/discussions-and-proposals#65 we moved ESLint/Prettier config to its own package inside packages directory.
At this moment we had to left all `eslint` dependencies in root `package.json` since `eslint-config-react-native` is not published yet.

[General] [Changed] - Move eslint/prettier config to packages/eslint-config-react-native
Pull Request resolved: #23688

Differential Revision: D14277068

Pulled By: cpojer

fbshipit-source-id: ef2d0b33210418318cc8324f15fedd84df0ef64d
@kelset kelset mentioned this issue Mar 20, 2019
Closed
@matei-radu matei-radu mentioned this issue Mar 23, 2019
Closed
@cpojer
Copy link
Member

cpojer commented Apr 24, 2019

We now have an eslint plugin that is shared across some projects. Let's make sure all projects adopt it.

@kelset
Copy link
Member

kelset commented Jun 30, 2019

Closing this in favour of the issue section of https://github.com/react-native-community/.github/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc where we are tackling this in more details

@kelset kelset closed this as completed Jun 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@cpojer @elicwhite @SimenB @grabbou @michalchudziak @mmazzarolo @kelset