[babel-plugin] Using dynamic styles with undefined values doesn't match expectations

[necolas]

Describe the issue

We encountered a bug internally that was root-caused to how StyleX has implemented dynamic styles.

The issue manifests when the value used with dynamic styles is undefined or null, which results in no value being set for the custom property.

const styles = stylex.create({
  width: (width: ?number) => ({
    width
  })
});

styles.width(undefined)

The browser does not fallback to a non-var value when the var is undefined (https://codepen.io/necolas/pen/VYvvBmj/). However, in StyleX we don't set the non-dynamic class names on the element anyway, because during a property merge the class for width is set to the one using the custom property.

You can see that here, where there is no logic checking for undefined values to decide which block of compiled styles to use. It's always the same block.

https://github.com/facebook/stylex/blob/0.14.2/packages/%40stylexjs/babel-plugin/tests/transform-stylex-create-test.js#L2032-L2053

When using inline styles with React DOM, if the value is undefined it doesn't get set and whatever value is present in your static CSS will get used. So to match this expectation is StyleX, we likely need to include more logic in the compiled output for dynamic styles, so that the relevant atomic class is not included if the input value is undefined.

We've previously discussed dynamic styles in the context of optimizing them, but the solution there should also be the foundation for fixing this bug: https://github.com/facebook/stylex/issues/737

Expected behavior

See above

Steps to reproduce

See above

Test case

See above

Additional comments

No response

[mellyeliu]

yea, I believe this behaviour is unintentional and inconsistent, we have other tests where the classname is appropriately wrapped with null (https://github.com/facebook/stylex/blob/0.14.2/packages/%40stylexjs/babel-plugin/tests/transform-stylex-create-test.js#L2156-L2168).

I was eyeing this part of dynamic styles classname generation as strange for a few reasons. The issue is due to a pretty brittle reverse lookup of dynamic styles in the create visitor which results in classnames not actually being wrapped in null as expected. Put up a rewrite of the dynamic styles logic that walks through the styles and properly detects static vs dynamic styles that should fix the issue: https://github.com/facebook/stylex/pull/1153

https://github.com/facebook/stylex/issues/737 would still be a good follow up to https://github.com/facebook/stylex/pull/1153 at some point

[nmn]

This bug was discovered previously and fixed. The css variable should not be set to anything and the actual property should be set to undefined as well which should get skipped.

Specifically, the clasNames should themselves be applied conditionally which is still the case in other tests, but in the linked example, the classNames are being applied in all scenarios which is the source of the bug.

[mellyeliu]

@nmn can you clarify? I think this is a new bug whereorigClassPaths lookup is broken. (maybe when hashing keys, the tests with the -- keys are still working as expected. didn't triage this closely). but tackled in https://github.com/facebook/stylex/pull/1153

[nmn]

@mellyeliu oh! I see the tests and that seems to be the case. I spot checked the tests and still saw the ternary expression, but many don’t have the ternary because the keys are now hashed.