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

feat: overhaul react native package to use expo universe as base #231

Merged
merged 11 commits into from
Nov 16, 2023

Conversation

petrkonecny2
Copy link
Contributor

@petrkonecny2 petrkonecny2 commented Nov 15, 2023

Why

We are not able to use this config inside react native projects. As we want to address this issue at the source rather than have to override a lot of rules on local level we feel this is the right way to go.

After carefully going through the STRV eslint packages and analyzing what overrides we need to add to each STRV react native project we feel React Native is so distinct from ReactJS and there are so many overrides that we need a separate base for the rules, one that is specifically made for React Native.

Also because React Native is still developing technology and thinks are still often changing we would like a rule set that can be kept up to date without having to maintain all of the rules ourselves.

How

As Expo has a really good eslint config that is used across all sorts of projects we feel like it is a good base that we can build on. We want to just keep couple of rules on top of it that would make it more alligned to other STRV configs.

* @author André Nanninga <andre.nanninga@strv.com>
* @copyright 2019 STRV
* @author Petr Konecny <petr.konecny@strv.com>
* @copyright 2016-present Expo, 2023-present STRV
Copy link
Member

Choose a reason for hiding this comment

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

Since this file was 100% authored by people contractually bound or directly employed by STRV and does not contain verbatim copies of any 3rd party code I think the copyright should belong to STRV in full.

@robertrossmann
Copy link
Member

Early review feedback: I am 100% onboard this change, but I would be much happier if we preserve as much coding style rules from the react ruleset as possible for maximum code authoring similarity. Of course, rules that cause you trouble because React Native requires you to write the code in a specific way are exempt from this wish. ❤️

Such rules are usually found in the */style.js rulesets but some coding style preferences might be included in the core rulesets but those generally also have more substantial side effects than just purely stylistic aspect.

@petrkonecny2
Copy link
Contributor Author

petrkonecny2 commented Nov 15, 2023

Early review feedback: I am 100% onboard this change, but I would be much happier if we preserve as much coding style rules from the react ruleset as possible for maximum code authoring similarity. Of course, rules that cause you trouble because React Native requires you to write the code in a specific way are exempt from this wish. ❤️

Such rules are usually found in the */style.js rulesets but some coding style preferences might be included in the core rulesets but those generally also have more substantial side effects than just purely stylistic aspect.

Thanks for the quick feedback 👍 So I have tried to also extend with @strv/eslint-config-react/style as you have suggested and it seems to work great. Except three rules that are causing issues: import/exports-last, import/group-exports, arrow-body-style from @strv/esling-config-base/style which then gets inherited by react/style.

These are all turned off in @strv/eslint-config-react but if you are not careful with the order you put the extends in and put style after react they get turned back on.

Should I also add these off rules into @strv/eeslint-config-react/style or should I keep them in the react native package only?

@robertrossmann
Copy link
Member

@petrkonecny2 Given the fact that these rules are enabled in the base/style.js config I think it is incorrect that they are then overridden/disabled in react/index.js config. Instead, it seems more appropriate they be disabled also in react/style.js but for this I'll defer to @matejpolak for final go on that change. If that change is wanted then I believe you can move those react overrides over to style.js in this PR and then you will not have trouble with them in react native config and won't need another override for them there. 🤞

FYI the intention is that even for react native projects, you only ever need to import the react native configs and any relevant react configs should be automatically included. At least that's how I would like things to be. 😇 Usually the import order I always suggest is the main ruleset, then optionally 😇 the /optional.js ruleset and finally, if desired by developers, the /style.js ruleset. In practice, then, any .eslintrc.js file should only ever need at most three extends entries.

If you find this not to be true for react native ruleset feel free to update it accordingly but don't worry if that's too out of scope of your current effort. 🚀

@matejpolak
Copy link
Contributor

Thanks a lot for the feedback @robertrossmann! I completely agree with moving these rules from react/index into the react/style.

@petrkonecny2 showed me how the linter behaves with the basic react/style extend, and we find it quite okay, I would definitely move forward with that. We added some more rules and I guess we are good to go 💪 🚀

@petrkonecny2 petrkonecny2 marked this pull request as ready for review November 16, 2023 12:26
@petrkonecny2
Copy link
Contributor Author

petrkonecny2 commented Nov 16, 2023

@robertrossmann @matejpolak I have included all feedback and it is now pretty much ready to merge for me.
Can you please take a look?

@petrkonecny2 petrkonecny2 changed the title feat(react-native): react native eslint package overhaul feat: overhaul react native package to use expo universe as base Nov 16, 2023
@matejpolak matejpolak merged commit 93218ef into master Nov 16, 2023
3 checks passed
@matejpolak matejpolak deleted the feat/rn_config branch November 16, 2023 15:09
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.

3 participants