strvcom / code-quality-tools

Monorepo with some frequently-used configurations we use on projects 🎨
BSD 3-Clause "New" or "Revised" License
91 stars 13 forks source link

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

Closed petrkonecny2 closed 12 months ago

petrkonecny2 commented 12 months ago

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.

robertrossmann commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

@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 commented 12 months ago

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 commented 12 months ago

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