[eslint] no-conflicting-props rule

[henryqdineen]

What changed / motivation ?

Never apply className and style properties alongside a {...stylex.props(...)} spread

I saw the above recommendation in the draft LLM context PR (#1345) and I thought it made sense as an ESLint rule.

This PR adds a new no-conflicting-props ESLint rule which disallows usage of className and style props on the same JSX element as a {...stylex.props(...)} spread, both before and after the spread. I don't have a great sense for how often people might run into this footgun but it seemed easy enough to statically analyze and imakes sense as an ESLint rule.

I considered that we might want to have the rule disallow JSX spreads after stylex.props (e.g <div {...stylex.props(...)} {...rest}> in case the rest has className or style but I did not include that in this PR.

Notes

• The estree types that are used in this and other ESLint rules don't have JSX types so I had to roll my own. I considered adding a dev dependency on hermes-estree, which seems to have a more complete set of types but I noticed that the JSXAttribute.name type was not accurate (or at least consistent with @babel/types and ast-types).

Pre-flight checklist

• I have read the contributing guidelines
  Contribution Guidelines
• Performed a self-review of my code

[mellyeliu]

This is quite useful to add within OSS - we have an internal version of this rule already (we are behind in consolidating all our lint rules!).

Before we merge, let's add in documentation within the README.md as a minimum

[mellyeliu]

can we add a test that checks for a conflict via another spread?

[henryqdineen]

What are you thinking by "conflict via another spread"? With the current implementation these would be false negatives:

<div {...stylex.props(styles.main)} {...{ className: 'foo' }} />
<div {...stylex.props(styles.main)} {...objThatHasClassName} />

The first example just seems unlikely and the second one seems difficult to implement without deeper analysis and/or type awareness. The simplest thing I can think of is to enforce that the stylex.props spread is the last spread so it overwrites previous conflicting props.

Let me know what you were thinking. Thanks!

[mellyeliu]

The inline spread, the latter is probably overkill. We've seen some ugly jsx before (though uncommon!) and it's an easy check.

We shouldn't enforce stylex.props being last, since the goal of this guidance is to avoid multiple sources of truth for className and style altogether.

[nmn]

We shouldn't enforce stylex.props being last

It might make sense to enforce that it is the last spread?

---

Separately, I was thinking of an enhancement to Babel to check for cases like:

<div {...props} {...stylex.props} />

And logging an error in dev if props contains className or style?

[henryqdineen]

Thanks. I pushed a commit in e6fa4b8 to handle the inline spread (e.g {...{className: 'foo'}). I considered handling stuff like {...{...{className: 'foo'}} but it seemed like an extreme edge case.

[mellyeliu]

It might make sense to enforce that it is the last spread?

Personally, would rather just disallow the pattern altogether than patch over the problem to have StyleX overwrite things when we don't know the author's intent. But no strong opinion.

Separately, I was thinking of an enhancement to Babel

Yeah good call! We should add more compiler validation in general.

[mellyeliu]

Happy with this, good to go aside from Flow

[henryqdineen]

Happy with this, good to go aside from Flow

Thanks. I fixed flow in b335eb0