feat: partial-file transforms with cascade-conflict guard#387
Open
feat: partial-file transforms with cascade-conflict guard#387
Conversation
Per-decl bails (state.markBail / bailUnsupported) now mark only the current declaration as skipTransform and roll back its partial state, so other declarations in the same file still convert normally. Downstream steps skip emission, JSX rewriting, and wrapper emission for skipped decls, and the styled-components default import is preserved when any decl stays. To preserve cascade semantics, detect-partial-cascade-conflict bails the whole file when a non-leaf (base that is extended) converted but the leaf that extends it could not — mixing StyleX atomic classes on a base with styled-components single-class overrides on a derived would make the derived's overrides unreliable. https://claude.ai/code/session_01RYpWvi5D46JBopQxw98uci
Contributor
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76bc61a540
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Extract processOneDecl() from the lower-rules loop for a cleaner outcome
("ok" | "skip" | "bail") and drop the unused skippedDecls tracking.
- Bail the whole file if any `css\`\`` helper can't be lowered: its source
was already removed by extractCssHelpersStep, so consumers would dangle.
- Preserve any named styled-components import (not just `styled`) that a
surviving skipped decl still references; removes a silent breakage when a
skipped decl uses `css\`\`` or similar inline.
- Add focused tests for warning emission, named-import preservation, and
the cascade-conflict bail path.
- Simplify detect-partial-cascade-conflict to a single pass.
https://claude.ai/code/session_01RYpWvi5D46JBopQxw98uci
- Partial migration: detect an existing top-level `const X = stylex.create({...})`
and append new entries into it instead of emitting a second `stylexStyles`
declaration. Falls back to the prior `stylexStyles` name when a new key would
collide with an existing one. Adds partial-alreadyHasStylex fixture.
- Address PR review P2: preserve `import { styled as sc }` aliases. Re-emit the
specifier with the original `imported` name so `import { sc }` (invalid) isn't
produced, while keeping the `local` alias that surviving decls reference.
- Add tests for the merge behaviour, key-collision fallback, and alias preservation.
- Update cross-file-prepass expectation to match the new single-object output.
https://claude.ai/code/session_01RYpWvi5D46JBopQxw98uci
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f71c6e4115
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Defer the merge-target decision to the END of analyzeBeforeEmitStep so every
emit-time key (staticBooleanVariants, callSiteCombinedStyles, promotedStyleProps,
variant keys) participates in the collision check. Previously only the keys
in resolvedStyleObjects at the top of the step were compared, which could
silently overwrite an existing entry when later-injected keys happened to match.
- Reject the merge target when its binding name is shadowed anywhere else in the
file (nested const / function declaration). Rewritten `sx={name.key}` references
would otherwise bind to the shadowing scope and produce incorrect output.
- Add tests covering the shadowed-name fallback.
https://claude.ai/code/session_01RYpWvi5D46JBopQxw98uci
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43de9e3695
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Detect parameter shadowing when picking an existing stylex.create merge target.
The shadow check now walks all j.Function params (FunctionDeclaration,
FunctionExpression, ArrowFunctionExpression, methods) including destructuring
patterns. Previously a parameter named `styles` wouldn't be caught and
`sx={styles.key}` would bind to the parameter instead of the top-level object.
- Bail the file when a skipped decl's template still interpolates an extracted
css helper (`${helper}` inside a skipped styled component). The helper's
source declaration was removed by extractCssHelpersStep, so the preserved
skipped decl would reference an undefined identifier.
- Stop pruning shared mixin/helper style keys when a skipped decl lists them
in extraStyleKeys/extraStyleKeysAfterBase/extendsStyleKey. Those keys are
owned by other decls; a transformed decl that also references them would
otherwise lose its stylex.create entry silently.
- Add regression tests: parameter-shadow fallback to stylexStyles, dangling-helper
bail, shared-mixin survival across mixed skipped/transformed consumers.
https://claude.ai/code/session_01RYpWvi5D46JBopQxw98uci
… wrappers The cascade-safety rule was inverted. Correct direction: - "styled-components restyling StyleX" is fine — styled-components CSS is injected at runtime AFTER StyleX's precompiled atomic CSS, so a styled-components leaf wrapping a StyleX base has its overrides win. - "StyleX restyling styled-components" is unsafe — StyleX classes carry the leaf overrides but the base's styled-components CSS injects later and can win on property conflicts the leaf was meant to override. Changes: - detect-partial-cascade-conflict now bails when a LEAF converts but its NON-LEAF base is skipped (previously the rule was inverted). - When a non-leaf base is transformed while a skipped leaf still references it via `styled(Base)\`...\``, force Base to emit as a wrapper function (not inlined) so the preserved leaf has a callable React component to reference and styled-components can inject its className onto it. - Build `extendedBy` from the full decl list (incl. skipped) so wrapper / supportsExternalStyles decisions account for preserved leaves. - analyze-after-emit's "clear supportsExternalStyles when no child delegates" check now considers skipped delegates so the wrapper forwards className/style. - Rename fixtures to match the corrected direction: partial-leafOnly → _unsupported.partial-leafConvertsBaseStays (now bails) _unsupported.partial-nonleafConvertsLeafSkips → partial-nonleafOnly (now converts) - Update the warning text and unit tests. https://claude.ai/code/session_01RYpWvi5D46JBopQxw98uci
Partial migration is now opt-in via `allowPartialMigration: true` on both `runTransform` and the per-file `TransformOptions`. The default (`false`) preserves the pre-flag behavior: any per-decl bail escalates to a whole-file bail. - Plumb the flag through run.ts → jscodeshift options → TransformContext. - lower-rules step: if allowPartialMigration is false and any decl was skipped, bail the whole file. - Test harness: auto-enable the flag for partial-* fixtures and the inline partial-file-transforms describe block; keep all other tests on the default. - Add a test that exercises the default-bail + opt-in-partial toggle. https://claude.ai/code/session_01RYpWvi5D46JBopQxw98uci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Per-decl bails (state.markBail / bailUnsupported) now mark only the current
declaration as skipTransform and roll back its partial state, so other
declarations in the same file still convert normally. Downstream steps skip
emission, JSX rewriting, and wrapper emission for skipped decls, and the
styled-components default import is preserved when any decl stays.
To preserve cascade semantics, detect-partial-cascade-conflict bails the
whole file when a non-leaf (base that is extended) converted but the leaf
that extends it could not — mixing StyleX atomic classes on a base with
styled-components single-class overrides on a derived would make the
derived's overrides unreliable.
https://claude.ai/code/session_01RYpWvi5D46JBopQxw98uci