Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/@stylexjs/eslint-plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -252,3 +252,24 @@ specifications.
"validImports": ["stylex", "@stylexjs/stylex"]
}
```

### `@stylexjs/no-conflicting-props`

This rule disallows using `className` or `style` props on elements that spread
`stylex.props()` to avoid conflicts and unexpected behavior.

#### Invalid examples

```jsx
<div {...stylex.props(styles.foo)} className="extra" />

<div {...stylex.props(styles.foo)} style={{ color: 'red' }} />
```

#### Config options

```json
{
"validImports": ["stylex", "@stylexjs/stylex"]
}
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,308 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

'use strict';

jest.disableAutomock();

const { RuleTester: ESLintTester } = require('eslint');
const rule = require('../src/stylex-no-conflicting-props');

const eslintTester = new ESLintTester({
parser: require.resolve('hermes-eslint'),
parserOptions: {
ecmaVersion: 6,
sourceType: 'module',
ecmaFeatures: {
jsx: true,
},
},
});

eslintTester.run('stylex-no-conflicting-props', rule.default, {
valid: [
{
code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...stylex.props(styles.main)} />;
}
`,
},
{
code: `
import * as stylex from '@stylexjs/stylex';
function Component() {
return <div className="foo" />;
}
`,
},
{
code: `
import * as stylex from '@stylexjs/stylex';
function Component() {
return <div style={{ color: 'red' }} />;
}
`,
},
{
code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...stylex.props(styles.main)} data-testid="test" />;
}
`,
},
{
code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...otherProps} className="foo" />;
}
`,
},
{
code: `
import { props, create } from '@stylexjs/stylex';
const styles = create({
main: { color: 'red' },
});
function Component() {
return <div {...props(styles.main)} />;
}
`,
},
{
options: [{ validImports: ['custom-stylex'] }],
code: `
import * as stylex from 'custom-stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...stylex.props(styles.main)} />;
}
`,
},
{
options: [{ validImports: [{ from: 'a', as: 'css' }] }],
code: `
import { css } from 'a';
const styles = css.create({
main: { color: 'red' },
});
function Component() {
return <div {...css.props(styles.main)} />;
}
`,
},
],
invalid: [
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...stylex.props(styles.main)} className="foo" />;
}
`,
errors: [
{
message:
'The `className` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...stylex.props(styles.main)} style={{ margin: 10 }} />;
}
`,
errors: [
{
message:
'The `style` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div className="foo" {...stylex.props(styles.main)} />;
}
`,
errors: [
{
message:
'The `className` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div style={{ margin: 10 }} {...stylex.props(styles.main)} />;
}
`,
errors: [
{
message:
'The `style` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
code: `
import { props, create } from '@stylexjs/stylex';
const styles = create({
main: { color: 'red' },
});
function Component() {
return <div {...props(styles.main)} className="foo" />;
}
`,
errors: [
{
message:
'The `className` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
code: `
import { props as p, create } from '@stylexjs/stylex';
const styles = create({
main: { color: 'red' },
});
function Component() {
return <div {...p(styles.main)} className="foo" />;
}
`,
errors: [
{
message:
'The `className` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
options: [{ validImports: ['custom-stylex'] }],
code: `
import * as stylex from 'custom-stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...stylex.props(styles.main)} className="foo" />;
}
`,
errors: [
{
message:
'The `className` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
options: [{ validImports: [{ from: 'a', as: 'css' }] }],
code: `
import { css } from 'a';
const styles = css.create({
main: { color: 'red' },
});
function Component() {
return <div {...css.props(styles.main)} className="foo" />;
}
`,
errors: [
{
message:
'The `className` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...stylex.props(styles.main)} className="foo" style={{ margin: 10 }} />;
}
`,
errors: [
{
message:
'The `className` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
{
message:
'The `style` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...stylex.props(styles.main)} {...{ className: 'foo' }} />;
}
`,
errors: [
{
message:
'The `className` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
{
code: `
import * as stylex from '@stylexjs/stylex';
const styles = stylex.create({
main: { color: 'red' },
});
function Component() {
return <div {...stylex.props(styles.main)} {...{ style: { margin: 10 } }} />;
}
`,
errors: [
{
message:
'The `style` prop should not be used when spreading `stylex.props()` to avoid conflicts.',
},
],
},
],
});
3 changes: 3 additions & 0 deletions packages/@stylexjs/eslint-plugin/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import enforceExtension from './stylex-enforce-extension';
import noLegacyContextualStyles from './stylex-no-legacy-contextual-styles';
import noLookaheadSelectors from './stylex-no-lookahead-selectors';
import noNonStandardStyles from './stylex-no-nonstandard-styles';
import noConflictingProps from './stylex-no-conflicting-props';
import noUnused from './stylex-no-unused';
import sortKeys from './stylex-sort-keys';
import validShorthands from './stylex-valid-shorthands';
Expand All @@ -21,6 +22,7 @@ const rules: {
'no-legacy-contextual-styles': typeof noLegacyContextualStyles,
'no-lookahead-selectors': typeof noLookaheadSelectors,
'no-nonstandard-styles': typeof noNonStandardStyles,
'no-conflicting-props': typeof noConflictingProps,
'no-unused': typeof noUnused,
'sort-keys': typeof sortKeys,
'valid-shorthands': typeof validShorthands,
Expand All @@ -30,6 +32,7 @@ const rules: {
'no-legacy-contextual-styles': noLegacyContextualStyles,
'no-lookahead-selectors': noLookaheadSelectors,
'no-nonstandard-styles': noNonStandardStyles,
'no-conflicting-props': noConflictingProps,
'no-unused': noUnused,
'sort-keys': sortKeys,
'valid-shorthands': validShorthands,
Expand Down
Loading
Loading