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

react/jsx-curly-spacing not working #857

Closed
tleunen opened this issue Sep 21, 2016 · 36 comments
Closed

react/jsx-curly-spacing not working #857

tleunen opened this issue Sep 21, 2016 · 36 comments

Comments

@tleunen
Copy link

tleunen commented Sep 21, 2016

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
Copy link
Member

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

@tleunen
Copy link
Author

tleunen commented Sep 21, 2016

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
Copy link
Member

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

@ljharb
Copy link
Member

ljharb commented Sep 21, 2016

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
Copy link
Author

tleunen commented Sep 22, 2016

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
Copy link

Same issue :-)

@carlows
Copy link

carlows commented Oct 24, 2016

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

<Comp>
  {something}
</Comp>

@ljharb
Copy link
Member

ljharb commented Oct 24, 2016

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

@zimme
Copy link

zimme commented Oct 31, 2016

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
Copy link

Kronuz commented Feb 7, 2017

Is there currently no rule to support this?

@ljharb
Copy link
Member

ljharb commented Feb 7, 2017

@Kronuz no. #857 (comment)

@fatfisz
Copy link
Contributor

fatfisz commented May 5, 2017

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
Copy link
Member

ljharb commented May 5, 2017

@fatfisz #857 (comment)

@fatfisz
Copy link
Contributor

fatfisz commented May 5, 2017

Thanks for the pointer, I missed that.

@fatfisz
Copy link
Contributor

fatfisz commented May 6, 2017

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
Copy link
Member

ljharb commented May 6, 2017

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
Copy link
Contributor

fatfisz commented May 6, 2017

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
Copy link
Member

ljharb commented May 6, 2017

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
Copy link
Contributor

fatfisz commented May 6, 2017

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

@ljharb
Copy link
Member

ljharb commented May 6, 2017

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

?

@fatfisz
Copy link
Contributor

fatfisz commented May 6, 2017

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
Copy link
Member

ljharb commented May 6, 2017

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
Copy link
Contributor

fatfisz commented May 6, 2017

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
Copy link
Member

ljharb commented May 6, 2017

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
Copy link
Contributor

fatfisz commented May 6, 2017

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

@fatfisz
Copy link
Contributor

fatfisz commented May 6, 2017

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
Copy link
Contributor

fatfisz commented May 6, 2017

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

@fatfisz
Copy link
Contributor

fatfisz commented May 6, 2017

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
Copy link
Member

ljharb commented May 7, 2017

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
Copy link
Contributor

fatfisz commented May 7, 2017

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

@fatfisz
Copy link
Contributor

fatfisz commented May 7, 2017

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
Copy link
Member

ljharb commented May 7, 2017

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

@fatfisz
Copy link
Contributor

fatfisz commented May 7, 2017

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
Copy link
Member

ljharb commented Jun 11, 2017

Fixed in #1177

@prewk
Copy link

prewk commented Jun 12, 2017

Awesome! Will you publish sometime today?

@ljharb
Copy link
Member

ljharb commented Jun 12, 2017

That's up to @yannickcr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

9 participants