Inverts the ESLint rule that forces us to remove padding within curly braces of object literals

[Golodhros]

This rule is a stylistic requirement that forces us to remove the padding between values of an object literal, and that mainly applies when we are destructuring values from an object or in parameter destructuring in the signature of a function.

The proposed change is making it the contrary, that means, to force us to add padding with spaces on this cases. The benefits I consider this brings are twofold:

First, we improve the readability of our code, being:

 let { velocity } = parameters;
 const fn = ({ velocity }) => velocity;

More readable than:

 let {velocity} = parameters;
 const fn = ({velocity}) => velocity;

Second, we are more in sync with what the JavaScript community in general does, as this rule is set up the proposed way in a code style guide as important as the AirBnB one. Also, a lot of examples out there use the padded version.

In this case, I understand this is more a stylistic proposal, although I really think the readability benefits are real.

[eb-hernan]

I kinda prefer the way we have right now

[davidp-eb]

In my case, I think we can just remove the rule so the developer can have the freedom of using it as it prefers, by the way, If I have to choose I prefer to keep it as it is now.

[eb-ignacioviejo]

Agree, I think it improves readability and helps to distinguish between arrays and objects faster

[LucasPadovan]

I believe that the spaces are a good practice. I've been using them in a personal project and it really helps readability.

[rwholey-eb]

I like no space (maybe I'm just used to it) but I also feel like it's not a big deal either way, i'm with @davidp-eb in that this is a rule that we could take no stance on and let the developer choose

[benmvp]

I'm with @rwholey-eb & @eb-hernan in that I like it, but like Ryan I think I'm just used it. I can go either way.

However, I strongly believe that we should be consistent. So that means:

1/ That it should be either on or off. Things will get crazy when someone people use one way and others use another way. Then we'll have this implicit rule that an entire file has to be one way, but that won't always be enforced. Then folks will go in and change everything to be one way when it's mixed. It just causes unnecessary work for everyone.

2/ We have to do it everywhere. You just showed destructuring patterns, but it'd be in other places too, such as import statements, object, literals, etc.

import { CONSTANT_NAME } from './path/to/file';

const OBJ = { this: 'that', he: 'she' };

return (
    <Component prop={ variable } />
);

You're right that this probably more common than our current approach. It's something that can be automatically fixed, so it wouldn't be hard to switch the code. It's just something that we'd all have to re-learn. Not sure if the readability is that great to be worth the change, but maybe?

[randallkanna-eb]

I agree with @benmvp. Especially with Things will get crazy when someone. I like consistency. But I also like readability so I like the const OBJ = { this: 'that', he: 'she' };. I can go with what everyone agrees upon but I also think it should be a rule

[matthewdowns-eb]

Also agreeing with @benmvp. I can see the readability argument on both sides, so I don't have a particular preference either way. I tend to err on the side of community-driven conventions, but JSX and import statements are a good counterpoint.

I feel strongly that we should have a linter rule either way. "Leaving it up to the devs/teams" will make our codebase more fractured and less consistent.

[eb-ignacioviejo]

I agree this should be on a rule to keep consistency, and if we don't identify such big improvements to make the change, I'd just leave it like it is.

[kwelch-eb]

I don't think the effort required for this change is near the value gained by it. While the spaces are slightly more readable, in my opinion once you get to 2 properties in an object it should be expanded to multiple lines so the value of those spaces is immediately lost.

[jcreamer898]

My name is Jonathan, I'm new here. I can haz moar whitespaces?

In all seriousness tho I agree with @Golodhros. Whitespace makes the world go round.

[jcreamer898]

ESLint and Prettier can handle this in zero seconds flat.

[shamlet-eb]

I actually like having the whitespace too. I find it easier to read.

[miglesiasEB]

In Summary:
For the proposal: IIIIIIII
Against: IIIII
For removing rule altogether: III
I don't care, but there must be a rule: IIIII

[kwelch-eb]

@miglesiasEB How do you feel about making an announcement at FE Guild and we have a post here people can react to with their vote.

(ie. ❤️ = leave as is, 👍 = Add spaces, 👎 = Remove rule entirely, 😄 = don't care, need rule)

[sennab]

I prefer it as is without spaces.

[eb-namchi]

I don't care, but there must be a rule!

[matthewdowns-eb]

Thanks to Marcos for keeping this discussion alive!

+1 for Kyle's idea to posit this to a larger audience at FE guilde. As discussed previously in this thread, we also need to consider the "cost of change" aspects. It's easy enough to change the rule and eslint --fix, but we also want to consider other possible side effects (merge conflicts, unknowns, etc.)

[aidan-eb]

I like the spaces but only if it becomes the rule and we auto fix the code base to have the spaces included. I'm all in for consistency. Will say though, I'm not a big fan of how the spaces look in JSX props, but I'll take it. However, for imports and function signatures I'm a heavy plus 1.

[melissaroman-eb]

I do like consistency for this one. My personal preference is to have the { spaces } for readability but if the majority think we should keep it as is, then I think we should go with consistency over personal preference.

[fiona-eb]

I emoji-ed kyle's post but would prefer to leave the current rule as is and leave spaces out. Also 100% agree with Kyle's opinion above and want to add that the readability aspect of this is debatable I think people can vouch for both sides but I'd err on keeping things as is to avoid future conflicts that come up. It annoyed me working in old pages and lint would give me errors on code I've never touched because of newer lint rules.

[jonathancreamer-eb]

I really wanna get this merged. My build just failed on an eslint check because of this...

😂

[Golodhros]

I just need some time to merge and fix this!
If anyone has the bandwidth, please don't wait for me!

[jonathancreamer-eb]

Should we just change it to a warning so it won't break builds for now? @Golodhros

[Golodhros]

I understood from Ben that we don´t have the concept of warning, our warnings break the build too. But I could be wrong.

…

On Tue, Oct 23, 2018 at 6:43 AM Jonathan Creamer ***@***.***> wrote: Should we just change it to a warning so it won't break builds for now? @Golodhros <https://github.com/Golodhros> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#74 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALpcWENQSzVbcebIfwO5dsQTA-40kL6ks5unx0UgaJpZM4Udjto> .

-- *Marcos Iglesias, Software Engineer* @Golodhros <http://twitter.com/#!/@golodhros> | Facebook <http://www.facebook.com/marcos.iglesias.valle> | Linkedin <http://www.linkedin.com/in/marcosiglesias> www.marcosiglesias.com <http://www.marcosiglesias.com/?utm_source=email&utm_medium=signature&utm_campaign=Email%20Signature> | Britecharts <http://eventbrite.github.io/britecharts/>

[jonathancreamer-eb]

Then maybe we should just remove the rule altogether. Enable it locally to do the fixes, then re-enable it once we get it all sorted out?

[benmvp]

A couple of things of note:

1/ We would need to merge the rule here, then bump our versions of the configs in our repos. Most of our repos are out-of-date so it'll take some work to get everything sorted out with the new rules.

2/ When I'm pushing new code, I've gotten in the habit of doing yarn eslint --fix as basically a poor person's prettier. It handles all of the stylistic stuff I forgot (like dangling commas, bracket alignment, etc.)

3/ We used to allow warnings in eslint, but they were pointless because no one would clean them up. So then we had dozens of warnings that kept growing. The result is that it became hard to distinguish between real errors and just warnings, especially for non-JS devs. I don't believe there to be any value in warnings. It's the same reason we fail on the React prop type warnings.

4/ I'm up for whatever the group decides, but judging from the comments here there really isn't an overwhelming consensus either way. So I would suggest leaving it as-is.

[jonathancreamer-eb]

It seems like the people who want it change, REALLY want it changed vs the people who are all "meh". Haha. To me it just comes down to common practice and readability. Plus, it's super easy to fix with eslint.

[Golodhros]

Let´s merge this then!

I was worried that all the other builds would start breaking, but if we first need to bump the configurations, then there is no immediate problem, right?

Thanks for the tip Ben, I will start doing that too.

Well, there was only half of the supporters that didn´t want to change the rule. It is true that we never discussed the kind of agreement we should have to change these rules though.