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

[Blocked] Lint rule to enforce all stylex styles are used within JSX #736

Open
nmn opened this issue Oct 9, 2024 · 4 comments
Open

[Blocked] Lint rule to enforce all stylex styles are used within JSX #736

nmn opened this issue Oct 9, 2024 · 4 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@nmn
Copy link
Contributor

nmn commented Oct 9, 2024

A somewhat simple lint rule that enforces that all references to the styles created with stylex.create are only referenced within a JSX expression.

In the case of string element, it must always be in a {...stylex.props} and in the case of custom components, it can also be passed as a regular prop.

That's it. That's the whole lint rule.

PS. stylex.create declarations within an export statement should be considered valid.

@nmn nmn added enhancement New feature or request good first issue Good for newcomers labels Oct 9, 2024
@necolas
Copy link
Contributor

necolas commented Oct 9, 2024

Are you saying that this pattern would be linted against?

const stylesA = stylex.create({})
const stylesB = stylex.create({})

// Not in jsx
const style = [ stylesA, stylesB ]

@nmn
Copy link
Contributor Author

nmn commented Oct 9, 2024

@necolas Yes. The intention is to encourage developers to use && and other conditional queries directly within stylex.props, but this is definitely more on the stylistic side of things.

@necolas
Copy link
Contributor

necolas commented Oct 9, 2024

It's not uncommon to use arrays to combine styles or themes like this though. And it doesn't seem to have a downside. In fact, in some cases it can improve perf because the array is not recreated on every render. It's the manual version of what I proposed in #737 - a further optimization to that pattern would be to memoize the return value of the function so it is stable across renders. And it's also how people are likely to combine style and theme partials outside of components.

const colors = stylex.createTheme(colors, {...})
const spacing = stylex.createTheme(colors, {...})
export const theme = [ colors, spacing ]

@nmn
Copy link
Contributor Author

nmn commented Oct 9, 2024

@necolas The specific use-case is when all styles are defined and used locally within the file. In such scenarios, stylex.props can be compiled away.

Also, this would rule would not apply to themes and to exports.

But yes, in other cases this rule might just be annoyance though. Let me think if it's possible to make this rule more narrow and only discourage unnecessary indirection. It might also be possible to further improve the compiler to optimise more cases of stylex.props instead.

Marking as blocked until we resolve these concerns.

@nmn nmn changed the title Lint rule to enforce all stylex styles are used within JSX [Blocked] Lint rule to enforce all stylex styles are used within JSX Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants