Eslint-plugin-react: react/jsx-curly-spacing not working

[tleunen]

For some reasons, I can't get the jsx-curly-spacing rule to report errors.

After enabling it (using the default airbnb settings), I set several use case and nothing is reported.

The config has the following setting so all these cases should be reported, but they are not in my case.

'react/jsx-curly-spacing': ['error', 'never', { allowMultiline: true }],

{ something}
{something }
{ something }

[yannickcr]

The jsx-curly-spacing rule apply only to jsx attributes.

[tleunen]

Oh I see... I'll better read the doc next time.
But do you think it should also be applied to all jsx curly values?

<Comp>
  {something}
</Comp>

[yannickcr]

ESLint rules does not apply to {something} here ? If not we should support it in this rule imho.

[ljharb]

I'm relatively sure we have a rule that enforces this spacing in our airbnb config; it's different from object curly spacing because it's not object curlies. Not sure off the top of my head which rule tho.

[tleunen]

From my testing, the rule is not applied for the example I posted. I checked all the jsx rules and couldn't find another rule which check this.

[luokuning]

Same issue :-)

[carlows]

Same issue, is there any other rule that works for this?

<Comp>
  {something}
</Comp>

[ljharb]

I think that perhaps an option to jsx-curly-spacing that enabled checking of child curlies would be appropriate here?

[zimme]

Also this doesn't work.

<Button
  active={prevButtonStatus}
  // I expect the following to error because spacing around prevButtonText
  text={<span>{ prevButtonText }</span>}
  onClickHandler={expandButtonHandler}
  className={classNames('btn', 'btn-dots')}
/>

[Kronuz]

Is there currently no rule to support this?

[ljharb]

@Kronuz no. https://github.com/yannickcr/eslint-plugin-react/issues/857#issuecomment-255827739

[fatfisz]

I'd like to work on this during the weekend :)

Should this be an expansion of react/jsx-curly-spacing or a completely new rule?

[ljharb]

@fatfisz https://github.com/yannickcr/eslint-plugin-react/issues/857#issuecomment-255827739

[fatfisz]

Thanks for the pointer, I missed that.

[fatfisz]

I've got a few questions about the config:

1. Should a situation be allowed where there are different spacing rules for attributes and children? E.g. never add spaces in attributes and always add spaces in children.
2. Should a situation be allowed where spacing in the attributes is ignored and only checked in children? This is similar to the current situation, but with the roles reversed.

Seeing as some people were confused as to why this rule was not working with the child expressions maybe another option is not necessary for now? (so the answers would be "no" and "no") The additional option could be added later on if there was a need.

Otherwise the answers to those questions will help me get the shape of the new option right.

[ljharb]

Yes, children and attributes should be under their own options. Both should be separately configurable to be ignored, or spaces required, or spaces forbidden.

[fatfisz]

Ok, so how about this:

1. By default config is for both attributes and children.
2. If the user wants to configure attributes or children separately, they can pass an additional option object under a "children" or an "attributes" properties.

Examples:

1. This config never allows spaces either for attributes or children:

['warning', 'never']

1. This config never allows spaces for attributes, but completely ignores children:

['warning', 'never', {
  children: 'ignore'
}]

1. This config never allows spaces or newlines around attribute expressions, but requires spaces in children and allows multiline expressions in children:

['warning', 'never', {
  allowMultiline: false,
  children: ['always', { allowMultiline: true }]
}]

[ljharb]

By default, it should do precisely what it does now, to avoid a breaking change.

For number 3, I'd say "children" etc should either just be false, true, or an object with overrides for things like allowMultiline.

[fatfisz]

So how would config look for ignored attributes and "never" in children? I think that's the final piece of info I need.

[ljharb]

{
  allowMultiline: true,
  children: { allowMultiline: false }, // checked + overrides allowMultiline
  attributes: false, // ignored
}

{
  allowMultiline: false,
  children: false, // ignored
  attributes: true, // checked + inherits allowMultiline
}

?

[fatfisz]

Makes sense, I thought of ['never', { children: true, attributes: false }], so it's essentially what you've written in the first example. So "attributes" is true by default and "children" is false by default. They both have to be "always" or "never" at the same time.

Thanks for the patience! Will hopefully finish implementing this soon.

[ljharb]

Hmm, I don't think they both should have to be always or never at the same time.

Let's add a new schema instead that gets rid of the string option entirely, only allows an object with allowMultiline and "foo" (always or never), and also children and attributes overrides for both?

(Not yet sure what to call "foo")

[fatfisz]

Good idea :)

I will go with requireSpaces as a placeholder for now, but it doesn't read well with "never" for me.

In this case children: true means

children: {
  requireSpaces: 'never',  // The new "foo" property
  allowMultiline: true
}

The default for the whole thing becomes { attributes: true, children: false }.

[ljharb]

Maybe let's just call it "spaces"?

That sounds good. Although, to clarify, { allowMultiline: true, spaces: 'never', attributes; true, children: false } would be the default - "children" and "attributes" inherit.

[fatfisz]

Ok, let's go with that. And yes, the full defaults are this.

[fatfisz]

Btw. I think there was a small bug: when only "spacing" was set in the extended options, the following line was setting "allowMultiline" to undefined:

var multiline = context.options[1] ? context.options[1].allowMultiline : true;

Then in all the following conditions !multiline was being checked, which yielded a different result than the default true value would.

Now I'll be checking if the property exists on the config object (using the has package, I found it being used in other rules).

[fatfisz]

There were no tests for this behavior - I'll add some.

[fatfisz]

I think we should do something about the "spacing" property name, it's quite similar to the new "spaces" property. How about merging the "spacing" object into the config object?

[ljharb]

Ah, that's a good point. I think leaving spacing as a separately configurable item (in the single-object-config schema, this would be a top-level key), and I think we should come up with a better name for "foo" than "spaces" - how about when?

[fatfisz]

Ok, will change it to "when", sounds good.

[fatfisz]

Found another bug:

<App foo={
{ bar: true, baz: true }
} />;

This would get 2 errors using the default settings: There should be no space after '{' and There should be no space before '}'.

This is because conditions for the object literal spacing were written so that the second one was never reached:

          if (sourceCode.isSpaceBetweenTokens(first, second)) {
            reportNoBeginningSpace(node, first);
          } else if (!config.allowMultiline && isMultiline(first, second)) {
            reportNoBeginningNewline(node, first, config);
          }

If the first condition fails, isMultiline will return false - there is no newline.

Will fix this and add appropriate tests.

[ljharb]

It would be helpful if the bug fix is in a separate PR; or at least a separate commit.

[fatfisz]

Each of the bugfixes is in a separate commit indeed. Separate PRs would mean some work on the tests (they are the most time-consuming), but I could do them if this would help.

Edit: oops, the first fix is mixed in with other things, unfortunately.

[ljharb]

Fixed in #1177

[prewk]

Awesome! Will you publish sometime today?

[ljharb]

That's up to @yannickcr