Skip to content

Fix comma-operator bug that made shadow*/textShadow* deprecation warnings and conversion unconditional#2844

Open
KAMRONBEK wants to merge 1 commit into
necolas:masterfrom
KAMRONBEK:fix-shadow-deprecation-warning
Open

Fix comma-operator bug that made shadow*/textShadow* deprecation warnings and conversion unconditional#2844
KAMRONBEK wants to merge 1 commit into
necolas:masterfrom
KAMRONBEK:fix-shadow-deprecation-warning

Conversation

@KAMRONBEK

Copy link
Copy Markdown

Problem

Using textShadow* (or shadow*) style props triggers the deprecation warning "textShadow*" style props are deprecated. Use "textShadow". for every caller of preprocess, regardless of the shadow / textShadow option flags, and the conversion to boxShadow/textShadow also runs unconditionally.

Root cause

In packages/react-native-web/src/exports/StyleSheet/preprocess.js the two guards were written with the comma operator instead of a logical AND:

if (
  (options.shadow === true,
  style.shadowColor != null || ...)
) {

The comma operator evaluates and discards options.shadow === true, so the condition reduces to "style contains shadow props" and the flag introduced in 2a901e5 ("Once Image is implemented using createElement, we can disable the 'boxShadow' generation just for Image") never had any effect.

Fix

Use a logical AND so the conversion and its deprecation warning only run when the corresponding option is enabled:

if (
  options.shadow === true &&
  (style.shadowColor != null || ...)
) {

Behavior through the public StyleSheet API is unchanged (it passes { shadow: true, textShadow: true } by default); only direct preprocess callers that did not opt in stop warning/converting, as originally intended.

To be explicit about scope: the deprecation warning for shadow*/textShadow* through the public API is intentional and retained — those props are deprecated. This PR fixes the broken option flag, not the deprecation itself, so the warning in the issue's repro still fires by design. If removing the public-API warning is wanted too, that seems like a separate decision (the remove-deprecation-warnings branch deliberately kept these two).

Tests

  • Updated the existing shadow/textShadow conversion tests in preprocess-test.js to opt in via the option flags.
  • Added a deprecation warnings for shadow styles describe block asserting: no console.warn and no conversion when the flag is not set (previously false-positive paths), and that the warning still fires when the flag is set. The new no-warning tests fail without the fix.
  • jest (full dom + node suites), prettier --check, eslint, and flow all pass.

Refs #2782

The 'shadow' and 'textShadow' preprocess option flags were written with
a comma operator instead of a logical AND, so the flag was evaluated and
discarded. As a result the "shadow*"/"textShadow*" deprecation
warnings fired (and the conversion ran) for every style containing
those props, regardless of the caller's options.

Use a logical AND so the conversion and deprecation warning only run
when the corresponding option is enabled, as intended by 2a901e5.

Fix necolas#2782
@codesandbox-ci

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f66e9eb:

Sandbox Source
react-native-web-examples Configuration

@KAMRONBEK

Copy link
Copy Markdown
Author

@necolas TL;DR: the two shadow guards in StyleSheet/preprocess.js use a comma operator instead of &&(options.shadow === true, style.shadowColor != null || …) — so the options.shadow/options.textShadow flags are evaluated and discarded, and the deprecation warning + boxShadow/textShadow conversion run for every caller regardless of the flags. This makes the flags work as originally intended (2a901e5). Note the public StyleSheet API passes both flags as true, so the deprecation warning there is retained by design — this PR fixes the broken gate, not the deprecation policy. Tests fail without the fix; jest/flow/eslint/prettier all green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant