Full import/export handling#68
Open
nstepien wants to merge 1 commit into
Open
Conversation
Resolve interpolation values across modules through every ES module
import/export form:
- default and namespace imports (`import d from`, `import * as ns from`),
including `${ns.member}` and nested `${ns.inner.member}` access
- re-exports (`export { x } from`, default-as-name, name-as-default),
`export * from` aggregations, `export * as ns from`, and transitive
chains through barrel files
- `export default` of css tagged templates and static expressions
(literals, signed numbers, parenthesized/TS-assertion-wrapped forms)
- type-only imports/exports are ignored (statement-level and inline
modifiers, including `export type * from`)
- aliased tags (`import { css as styled } from 'ecij'`), with a scope
check so shadowed bindings are not treated as the tag
Resolution follows ESM semantics: explicit exports shadow `export *`
sources even when not statically resolvable, ambiguous star exports are
not resolved (warn instead of picking a source), and `export *` never
forwards the default export.
Hardening that came out of review:
- a pending-load wait graph prevents build deadlocks on cyclic and
self-referencing barrels while still awaiting concurrent siblings
- external modules in re-export chains are skipped instead of loaded
- class names from skipped extractions no longer leak into consumers
(cross-module and same-file, with forward references still resolving)
- stylesheet dependencies are only recorded for successful resolutions,
so probed-but-failing star sources no longer drag dead CSS in
- watch mode: traversed dependencies are registered via addWatchFile and
evicted from caches on watchChange
Tested with the Vite pipeline and with plain rolldown (which hands raw
TypeScript to the transform), 12 -> 24 tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR substantially refactors ecij's CSS extraction pipeline to support rich ESM module resolution. The core changes introduce richer type models for tracking imports and exports, helpers for static expression evaluation and cycle detection, and a comprehensive re-export resolution pipeline that follows ChangesESM Import/Export Resolver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/fixtures/sibling-tokens.ts (1)
1-1: ⚡ Quick winUse SCREAMING_SNAKE_CASE for module-level constant.
The exported constant
padshould useSCREAMING_SNAKE_CASEper the coding guidelines for this project. As per coding guidelines, "Use SCREAMING_SNAKE_CASE for module-level constants" applies to**/*.{ts,tsx,js}files.♻️ Proposed fix
-export const pad = 4; +export const PAD = 4;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fixtures/sibling-tokens.ts` at line 1, Rename the module-level exported constant `pad` to SCREAMING_SNAKE_CASE (`PAD`) and update its export declaration (`export const pad`) to `export const PAD`, then update all usages/imports in the codebase to reference `PAD` instead of `pad` (search for the symbol `pad` to find call sites), ensuring build and tests pass after the rename.Source: Coding guidelines
test/fixtures/self-barrel-tokens.ts (1)
1-1: ⚡ Quick winUse SCREAMING_SNAKE_CASE for module-level constant.
The exported constant
tokenColorshould useSCREAMING_SNAKE_CASEper the coding guidelines for this project. As per coding guidelines, "Use SCREAMING_SNAKE_CASE for module-level constants" applies to**/*.{ts,tsx,js}files.♻️ Proposed fix
-export const tokenColor = 'teal'; +export const TOKEN_COLOR = 'teal';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fixtures/self-barrel-tokens.ts` at line 1, The module-level constant tokenColor violates the SCREAMING_SNAKE_CASE rule; rename the exported symbol to TOKEN_COLOR and update every usage/import to match (change export const tokenColor -> export const TOKEN_COLOR and update any references/imports across the codebase, including barrels or tests that import tokenColor). Ensure TypeScript/ES exports and any default/barrel re-exports still resolve after renaming.Source: Coding guidelines
test/fixtures/typed.input.ts (1)
14-20: 💤 Low valueModule-level constant uses camelCase instead of SCREAMING_SNAKE_CASE.
The exported constant
usesTypedToneshould beUSES_TYPED_TONEper the guideline requiring SCREAMING_SNAKE_CASE for module-level constants. However, as with the other fixture files, this is a test fixture where readability and natural naming may take precedence. As per coding guidelines, "Use SCREAMING_SNAKE_CASE for module-level constants" applies to**/*.{ts,tsx,js}files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fixtures/typed.input.ts` around lines 14 - 20, The exported module-level constant usesTypedTone violates the SCREAMING_SNAKE_CASE rule; rename the symbol usesTypedTone to USES_TYPED_TONE and update any local references/usages in this fixture so the CSS template string remains the same but the exported identifier uses SCREAMING_SNAKE_CASE (ensure you change the export name wherever usesTypedTone is referenced).Source: Coding guidelines
test/fixtures/typed-decoy.ts (1)
3-3: 💤 Low valueModule-level constant uses camelCase instead of SCREAMING_SNAKE_CASE.
Per coding guidelines,
typedToneshould beTYPED_TONE(SCREAMING_SNAKE_CASE for module-level constants). However, since this is a test fixture where the export name is significant for import resolution testing, and renaming would cascade to all consumers, this may be an acceptable deviation. Consider whether test fixtures should follow relaxed naming rules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fixtures/typed-decoy.ts` at line 3, The module-level constant exported as typedTone violates the SCREAMING_SNAKE_CASE convention; either rename the export to TYPED_TONE to follow guidelines (update all imports/consumers accordingly) or, if this is an intentional test fixture exception, add a brief comment above the export explaining the deliberate deviation so reviewers know it’s intentional; locate the export named typedTone in the fixture and apply one of these two changes consistently.Source: Coding guidelines
test/fixtures/typed-tokens.ts (1)
1-1: 💤 Low valueModule-level constant uses camelCase instead of SCREAMING_SNAKE_CASE.
Per coding guidelines,
typedToneshould beTYPED_TONE. Same consideration as intyped-decoy.ts: this is a test fixture where the export name matters for resolution testing. As per coding guidelines, "Use SCREAMING_SNAKE_CASE for module-level constants" applies to**/*.{ts,tsx,js}files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fixtures/typed-tokens.ts` at line 1, The module-level constant is named typedTone but must follow SCREAMING_SNAKE_CASE; rename the export symbol typedTone to TYPED_TONE and update any consumers/tests that import or reference typedTone to use TYPED_TONE instead (match the change made in typed-decoy.ts style); ensure the exported value remains the same ('salmon') and that any resolution tests or fixtures referencing the original symbol are adjusted accordingly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index.ts`:
- Around line 1101-1183: The code mutates the shared stylesheetDependencies
while resolving interpolations in processInterpolatedDeclaration, causing
imports to be committed even when a declaration is later skipped or deferred;
fix by creating a per-declaration Set (e.g., localDependencies) inside
processInterpolatedDeclaration and use it for any dependency additions performed
by resolveValue/resolveMemberPath paths instead of mutating
stylesheetDependencies directly, ensure all early returns ('skipped' or
'deferred') do not touch the global set, and after
addProcessedDeclaration(declaration, cssContent) succeeds merge
localDependencies into the global stylesheetDependencies Set.
- Around line 426-438: isCssTagTemplate currently only checks scope.identifiers
and thus misses bindings declared later in the same scope; update it to also
scan the scope's AST block for any declarations that bind the same name
(node.tag.name) even if they haven't been added to scope.identifiers yet.
Specifically, inside isCssTagTemplate (and where you reference
scope.identifiers), inspect scope.block (or scope.node) for VariableDeclarator
ids, FunctionDeclaration ids, ClassDeclaration ids (and similar binding sites)
whose name equals node.tag.name and return false if any are found; keep the
existing parent-scope walk for earlier shadowing as well.
---
Nitpick comments:
In `@test/fixtures/self-barrel-tokens.ts`:
- Line 1: The module-level constant tokenColor violates the SCREAMING_SNAKE_CASE
rule; rename the exported symbol to TOKEN_COLOR and update every usage/import to
match (change export const tokenColor -> export const TOKEN_COLOR and update any
references/imports across the codebase, including barrels or tests that import
tokenColor). Ensure TypeScript/ES exports and any default/barrel re-exports
still resolve after renaming.
In `@test/fixtures/sibling-tokens.ts`:
- Line 1: Rename the module-level exported constant `pad` to
SCREAMING_SNAKE_CASE (`PAD`) and update its export declaration (`export const
pad`) to `export const PAD`, then update all usages/imports in the codebase to
reference `PAD` instead of `pad` (search for the symbol `pad` to find call
sites), ensuring build and tests pass after the rename.
In `@test/fixtures/typed-decoy.ts`:
- Line 3: The module-level constant exported as typedTone violates the
SCREAMING_SNAKE_CASE convention; either rename the export to TYPED_TONE to
follow guidelines (update all imports/consumers accordingly) or, if this is an
intentional test fixture exception, add a brief comment above the export
explaining the deliberate deviation so reviewers know it’s intentional; locate
the export named typedTone in the fixture and apply one of these two changes
consistently.
In `@test/fixtures/typed-tokens.ts`:
- Line 1: The module-level constant is named typedTone but must follow
SCREAMING_SNAKE_CASE; rename the export symbol typedTone to TYPED_TONE and
update any consumers/tests that import or reference typedTone to use TYPED_TONE
instead (match the change made in typed-decoy.ts style); ensure the exported
value remains the same ('salmon') and that any resolution tests or fixtures
referencing the original symbol are adjusted accordingly.
In `@test/fixtures/typed.input.ts`:
- Around line 14-20: The exported module-level constant usesTypedTone violates
the SCREAMING_SNAKE_CASE rule; rename the symbol usesTypedTone to
USES_TYPED_TONE and update any local references/usages in this fixture so the
CSS template string remains the same but the exported identifier uses
SCREAMING_SNAKE_CASE (ensure you change the export name wherever usesTypedTone
is referenced).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e19a3158-9acc-42d8-b238-2df58492bf4b
📒 Files selected for processing (56)
README.mdsrc/index.tstest/fixtures/ambiguous-a.tstest/fixtures/ambiguous-b.tstest/fixtures/ambiguous-barrel.tstest/fixtures/ambiguous.input.tstest/fixtures/barrel-a.tstest/fixtures/barrel-b.tstest/fixtures/barrel-c.tstest/fixtures/barrel-d.tstest/fixtures/barrel-deep-only.input.tstest/fixtures/barrel-nested.tstest/fixtures/barrel.input.tstest/fixtures/barrel.tstest/fixtures/broken-export.input.tstest/fixtures/broken-export.tstest/fixtures/cycle-a.input.tstest/fixtures/cycle-b.tstest/fixtures/default-passthrough-source.tstest/fixtures/default-passthrough.tstest/fixtures/export-default-css.tstest/fixtures/export-default-literal.tstest/fixtures/export-default-local.tstest/fixtures/export-default-negative.tstest/fixtures/export-default-wrapped-css.tstest/fixtures/external-barrel.tstest/fixtures/external-star.input.tstest/fixtures/external-tokens.tstest/fixtures/import-export-hardening.input.tstest/fixtures/import-export.input.tstest/fixtures/named-styles.tstest/fixtures/namespace-edge-cases.input.tstest/fixtures/ns-chain-star.tstest/fixtures/ns-chain.tstest/fixtures/reexports-named.tstest/fixtures/reexports-namespace.tstest/fixtures/reexports-star.tstest/fixtures/same-file-classes.input.tstest/fixtures/self-barrel-tokens.tstest/fixtures/self-barrel.input.tstest/fixtures/self-barrel.tstest/fixtures/sibling-consumer.tstest/fixtures/sibling-producer.tstest/fixtures/sibling-tokens.tstest/fixtures/siblings.input.tstest/fixtures/star-over-default.tstest/fixtures/star-precedence-a.tstest/fixtures/star-precedence.input.tstest/fixtures/star-precedence.tstest/fixtures/tag-alias.input.tstest/fixtures/typed-barrel.tstest/fixtures/typed-decoy.tstest/fixtures/typed-tokens.tstest/fixtures/typed.input.tstest/plugin.test.tsvitest.config.ts
Comment on lines
+426
to
+438
| function isCssTagTemplate(node: TaggedTemplateExpression, scope: Scope): boolean { | ||
| if (!(node.tag.type === 'Identifier' && cssTagNames.has(node.tag.name))) { | ||
| return false; | ||
| } | ||
|
|
||
| // A local binding shadowing the imported tag means this is not the ecij tag | ||
| for (let current: Scope | null = scope; current !== null; current = current.parent) { | ||
| if (current.identifiers.has(node.tag.name)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Shadow checks miss later bindings in the same scope.
isCssTagTemplate() only sees names already inserted into scope.identifiers, but those bindings are recorded when the visitor reaches their declarations later on (for example, Line 565 and Line 672). A later let/const/var/function/class styled = ... still shadows the imported alias from the start of the scope, so styled\...`can be extracted here even though runtime resolution hits a TDZ or hoisted local binding instead ofecij`'s tag.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 426 - 438, isCssTagTemplate currently only checks
scope.identifiers and thus misses bindings declared later in the same scope;
update it to also scan the scope's AST block for any declarations that bind the
same name (node.tag.name) even if they haven't been added to scope.identifiers
yet. Specifically, inside isCssTagTemplate (and where you reference
scope.identifiers), inspect scope.block (or scope.node) for VariableDeclarator
ids, FunctionDeclaration ids, ClassDeclaration ids (and similar binding sites)
whose name equals node.tag.name and return false if any are found; keep the
existing parent-scope walk for earlier shadowing as well.
Comment on lines
+1101
to
+1183
| async function processInterpolatedDeclaration( | ||
| declaration: Declaration, | ||
| finalAttempt: boolean, | ||
| ): Promise<'extracted' | 'deferred' | 'skipped'> { | ||
| const { quasis, expressions } = declaration.node.quasi; | ||
|
|
||
| let cssContent = ''; | ||
| let allResolved = true; | ||
|
|
||
| for (let i = 0; i < quasis.length; i++) { | ||
| cssContent += quasis[i]!.value.raw; | ||
|
|
||
| if (i < expressions.length) { | ||
| const expression = expressions[i]!; | ||
|
|
||
| let resolvedValue: string | undefined; | ||
|
|
||
| if ( | ||
| expression.type === 'Literal' && | ||
| (typeof expression.value === 'string' || typeof expression.value === 'number') | ||
| ) { | ||
| resolvedValue = String(expression.value); | ||
| } else if ( | ||
| expression.type === 'UnaryExpression' && | ||
| (expression.operator === '-' || expression.operator === '+') && | ||
| expression.argument.type === 'Literal' && | ||
| typeof expression.argument.value === 'number' | ||
| ) { | ||
| resolvedValue = String( | ||
| expression.operator === '-' ? -expression.argument.value : expression.argument.value, | ||
| ); | ||
| } else if (expression.type === 'Identifier') { | ||
| resolvedValue = await resolveValue(expression.name, declaration.scope); | ||
|
|
||
| if (resolvedValue === undefined) { | ||
| // Cannot resolve - skip this entire css`` block | ||
| const expression = unwrapExpression(expressions[i]!); | ||
|
|
||
| let resolvedValue = resolveStaticExpression(expression); | ||
|
|
||
| if (resolvedValue === undefined) { | ||
| const memberPath = | ||
| expression.type === 'MemberExpression' | ||
| ? flattenMemberExpressionPath(expression) | ||
| : undefined; | ||
|
|
||
| if (expression.type === 'Identifier') { | ||
| resolvedValue = await resolveValue(expression.name, declaration.scope); | ||
|
|
||
| // A class name of a same-file declaration that has not been | ||
| // extracted: defer in case it is a forward reference whose | ||
| // declaration is still pending; once no progress can be made | ||
| // it is a failed extraction and must not leak (no rule exists). | ||
| if ( | ||
| resolvedValue !== undefined && | ||
| ownClassNames.has(resolvedValue) && | ||
| !extractedClasses.has(resolvedValue) | ||
| ) { | ||
| if (!finalAttempt) return 'deferred'; | ||
| resolvedValue = undefined; | ||
| } | ||
|
|
||
| if (resolvedValue === undefined) { | ||
| // Cannot resolve - skip this entire css`` block | ||
| context.warn( | ||
| { | ||
| pluginCode: 'UNRESOLVED_INTERPOLATION', | ||
| message: `skipped CSS extraction — could not resolve "${expression.name}" to a static string or number`, | ||
| }, | ||
| expression.start, | ||
| ); | ||
| return 'skipped'; | ||
| } | ||
| } else if (memberPath !== undefined) { | ||
| // Namespace member access: `${ns.foo}` / `${ns.inner.foo}` | ||
| resolvedValue = await resolveMemberPath(memberPath, declaration.scope); | ||
|
|
||
| if (resolvedValue === undefined) { | ||
| context.warn( | ||
| { | ||
| pluginCode: 'UNRESOLVED_INTERPOLATION', | ||
| message: `skipped CSS extraction — could not resolve "${memberPath.join('.')}" to a static string or number`, | ||
| }, | ||
| expression.start, | ||
| ); | ||
| return 'skipped'; | ||
| } | ||
| } else { | ||
| // Complex expression - skip this entire css`` block | ||
| context.warn( | ||
| { | ||
| pluginCode: 'UNRESOLVED_INTERPOLATION', | ||
| message: `skipped CSS extraction — could not resolve "${expression.name}" to a static string or number`, | ||
| pluginCode: 'COMPLEX_INTERPOLATION', | ||
| message: | ||
| 'skipped CSS extraction — interpolation is not a static string, number, or identifier', | ||
| }, | ||
| expression.start, | ||
| ); | ||
| allResolved = false; | ||
| break; | ||
| return 'skipped'; | ||
| } | ||
| } else { | ||
| // Complex expression - skip this entire css`` block | ||
| context.warn( | ||
| { | ||
| pluginCode: 'COMPLEX_INTERPOLATION', | ||
| message: | ||
| 'skipped CSS extraction — interpolation is not a static string, number, or identifier', | ||
| }, | ||
| expression.start, | ||
| ); | ||
| allResolved = false; | ||
| break; | ||
| } | ||
|
|
||
| cssContent += resolvedValue; | ||
| } | ||
| } | ||
|
|
||
| // Only process if all interpolations were resolved | ||
| if (allResolved) { | ||
| addProcessedDeclaration(declaration, cssContent); | ||
| addProcessedDeclaration(declaration, cssContent); | ||
| return 'extracted'; |
There was a problem hiding this comment.
Only commit stylesheet dependencies after the declaration is emitted.
This path evaluates interpolations against the shared stylesheetDependencies set as it goes. If an early interpolation resolves through resolveExportValue()/resolveMemberPath() but a later one returns 'skipped' or 'deferred', the dependency import remains even though this declaration never reaches addProcessedDeclaration(). That can pull unrelated CSS into the final output. Keep a per-declaration dependency set and merge it only after extraction succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 1101 - 1183, The code mutates the shared
stylesheetDependencies while resolving interpolations in
processInterpolatedDeclaration, causing imports to be committed even when a
declaration is later skipped or deferred; fix by creating a per-declaration Set
(e.g., localDependencies) inside processInterpolatedDeclaration and use it for
any dependency additions performed by resolveValue/resolveMemberPath paths
instead of mutating stylesheetDependencies directly, ensure all early returns
('skipped' or 'deferred') do not touch the global set, and after
addProcessedDeclaration(declaration, cssContent) succeeds merge
localDependencies into the global stylesheetDependencies Set.
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
1 participant
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.
Summary
Removes the last big TODO: interpolation values inside
css\`` templates now resolve across modules through every ES module import/export form.Supported forms
import d from,import * as ns from), including${ns.member}and nested${ns.inner.member}accessexport { x } from,export { default as x } from,export { x as default } from,export * from(excludingdefault),export * as ns from, and transitive chains through barrel files — includingimport d from 'mod'; export { d };, which oxc normalizes to an entry pointing at the local binding rather thandefaultexport defaultof css tagged templates and static expressions (string/number literals, signed numbers, parenthesized / TS-assertion-wrapped forms) via a single shared static-expression evaluatorimport type { x }, inlineimport { type x },export type { x } from,export { type x } from, andexport type * fromimport { css as styled } from 'ecij'), with a scope check so local bindings shadowing the tag are not extractedESM-conformant resolution semantics
export *sources even when their value is not statically resolvable (warn instead of silently taking the star value)export *never forwards the default exportRobustness
context.loadwould close a wait cycle, so self-referencing barrels and mutual css-class cycles resolve (or degrade to a warning) instead of deadlocking the build, while concurrent sibling modules still await each other's extraction resultsaddWatchFile, andwatchChangeevicts the per-file cachesKnown limitations (documented in the README)
csstag itself must be imported directly from'ecij'; re-exporting the tag through another module leaves templates untransformedTest plan
export *chains and explicit-over-star precedence), default/namespace/re-export matrix, namespace edge cases, star ambiguity, cycles, concurrent siblings, externals, phantom classes, tag aliases, and type-only formsisTypehandling, since Vite strips types before plugin transforms runnpm test(multiple shuffle seeds),npm run typecheck,npm run format:check, andnpm run buildall pass🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
Bug Fixes