facebook / stylex

StyleX is the styling system for ambitious user interfaces.
https://stylexjs.com
MIT License
8.4k stars 310 forks source link

@stylexjs/eslint-plugin: 'Computed key cannot be resolved' when overwriting var #337

Open olivierpascal opened 9 months ago

olivierpascal commented 9 months ago

The problem

Computed key cannot be resolved.eslint(@stylexjs/valid-styles)

How to reproduce

// bug.stylex.ts

import * as stylex from '@stylexjs/stylex';

const vars = {
  color: 'red',
};

export const componentVars = stylex.defineVars(vars);
// bug.ts

import * as stylex from '@stylexjs/stylex';

import { componentVars } from './bug.stylex';

export const styles = stylex.create({
  host: {
    [componentVars.color]: 'blue',
//   ^^^^^^^^^^^^^^^^^^^
  },
});
nmn commented 9 months ago

This is a known bug, but thanks for the issue.

rjsdnql123 commented 9 months ago

@nmn I am experiencing a similar issue.

As far as I understand, stylex.defineVars in stylexjs is used to set and retrieve preset values. However, in the provided example, it seems that the values from stylex.defineVars are being used as the key for stylex.create.

If I understand correctly, this approach seems to deviate from the intended usage of stylexjs.


The issue I'm experiencing is as follows: I'm encountering the same 'eslint' error. Could you please let me know if using it in this way goes against the intended purpose of 'stylexjs'?


const styleKeySet = {
  color: 'color',
  background: 'background-color',
}

export const styles = stylex.create({
  host: {
    [styleKeySet.background]: preset.red,
  },
});
nmn commented 9 months ago

@rjsdnql123

As far as I understand, stylex.defineVars in stylexjs is used to set and retrieve preset values.

As the name suggests, defineVars is used to defining CSS variables and giving them a default value. These variables can be used as both values and keys since variables values can be overridden on an DOM element and all its children. createTheme exists to override an entire set of variables, but they can also be used in stylex.create for one-off overrides.

If I understand correctly, this approach seems to deviate from the intended usage of stylexjs.

This is not true, hopefully the explaination above makes sense?


Could you please let me know if using it in this way goes against the intended purpose of 'stylexjs'?

This example feels contrived. Why are you using an object to define keys and using them by reference rather than declaring keys inline within stylex.create? On it's surface, this pattern looks unnecessarily complicated and I would suggest against it.

Assuming there is a valid use-case for this, we generally only suggest using string constants and not object constants since objects can be mutated.


Please explain a bit further on why you need such a pattern so I can give you more details.

rjsdnql123 commented 9 months ago

@nmn

As the name suggests, defineVars is used to defining CSS variables and giving them a default value. These variables can be used as both values and keys since variables values can be overridden on an DOM element and all its children. createTheme exists to override an entire set of variables, but they can also be used in stylex.create for one-off overrides.

Can you show me an example?

necolas commented 9 months ago

Yeah it's confusing if they can be used for keys and values, since in CSS the value must be the property wrapped in the var() function rather than being the same on both sides. Plus we have createTheme for applying new values to existing variables

nmn commented 9 months ago

@rjsdnql123 Start with the theming section on the website. Then see the Card component in the NextJS example in the repo.

Usually, variables should not be used as keys. It's an escape hatch.

arthurur commented 9 months ago

@nmn What about media query tokens? Those will be used as keys, e.g. I'm not able to declare one media object with my breakpoint queries nested and use those as keys for a stlye property conditional value, such as:

width: {
  default: "150px",
  [media["bp-tablet"]]: "100px",
},
nmn commented 9 months ago

@arthurur We recommend using separate constants since objects are mutable in JS, however, the pattern you're describing should work. Was media declared as a top-level const in the same file? Can you share more details?

arthurur commented 9 months ago

It was not, originally it was a const declared on my tokens.stylex.ts. Though I tried to bring it down to the same component and the result is the same:

image
nmn commented 9 months ago

@arthurur

  1. You're using the const before it is defined.
  2. Once you move it up, it should work, although the ESLint rule will still give you an error. As I said we recommend multiple string constants and not objects, because objects are mutable.
olivierpascal commented 8 months ago

Sorry to reopen this @nmn but it seems not to be resolved.

// vars.stylex.ts

import stylex from '@stylexjs/stylex';

export const vars = stylex.defineVars({
  color: 'blue',
});
// page.tsx

import stylex from '@stylexjs/stylex';
import { vars } from './vars.stylex';

const styles = stylex.create({
  host: {
    // Error here: Computed key cannot be resolved. eslint(@stylexjs/valid-styles)
    [vars.color]: 'red',
  },
});
QingqiShi commented 8 months ago

I think the fix just hasn't been released yet.

nmn commented 7 months ago

Just to verify, @olivierpascal the issue you're describing is an issue with the ESLint rule? The compilation is working correctly, right?

olivierpascal commented 7 months ago

Correct!

QingqiShi commented 7 months ago

Just pulled the latest main branch to double-check this. I added @oliverpascal's exact example to the tests, and it indeed passes. Here's a screenshot showing the test passing.

image