Repository navigation
feat: unified loader query. - #56
knightedcodemonkey wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (74.41%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
- Coverage 91.56% 89.99% -1.58%
==========================================
Files 13 13
Lines 3700 4057 +357
Branches 647 713 +66
==========================================
+ Hits 3388 3651 +263
- Misses 284 376 +92
- Partials 28 30 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified &stable loader query flag as an additive alias for &combined&types, streamlining the developer experience for importing modules with CSS, stable selectors, and type generation support. The implementation extends the existing loader and type generation infrastructure to recognize this new flag alongside the existing query parameter combinations.
Changes:
- Added
&stablequery flag as an alias that behaves like&combined&typesand marks imports for type generation - Extended lexer to track named exports from modules for accurate type declaration generation
- Added comprehensive test coverage for the new stable query functionality
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/playwright/src/auto-stable/shadow-tree.tsx.knighted-css-stable.d.ts | Generated TypeScript declaration file for shadow-tree CSS module with stable selectors |
| packages/playwright/src/auto-stable/shadow-tree.tsx | Updated ShadowTree component to accept stableSelectors as a prop instead of importing directly |
| packages/playwright/src/auto-stable/lit-host.ts.knighted-css-stable.d.ts | Generated TypeScript declaration file for combined shadow-tree module import |
| packages/playwright/src/auto-stable/lit-host.ts | Updated to use &stable query flag and pass stableSelectors to ShadowTree component |
| packages/playwright/playwright.config.ts | Reduced test timeout from 20s to 5s |
| packages/css/test/loaderUnit.test.ts | Added test coverage for stable query behavior and determineSelectorVariant mapping |
| packages/css/test/loaderQueries.types.ts | Added type-level tests for stable query module signatures |
| packages/css/test/lexer.test.ts | Added assertions to verify export tracking in lexer |
| packages/css/test/generateTypes.test.ts | Added test for stable query import scanning and type generation |
| packages/css/src/loaderInternals.ts | Added STABLE_QUERY_FLAG constant and updated hasCombinedQuery to recognize stable flag |
| packages/css/src/loader.ts | Updated loader to treat stable query flag as requesting types |
| packages/css/src/lexer.ts | Extended module analysis to extract and track named exports from declarations |
| packages/css/src/generateTypes.ts | Added stable declaration generation logic including query parsing, export tracking, and declaration formatting |
| packages/css/loader-queries.d.ts | Added ambient type declarations for all stable query permutations |
| docs/combined-queries.md | Updated documentation to explain stable query flag as primary UX and types as legacy option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| declare module ${JSON.stringify(entry.specifier)} { | ||
| export const knightedCss: string | ||
| export const stableSelectors: Readonly<${selectorType}> | ||
| ${defaultBlock} ${exportsBlock} |
There was a problem hiding this comment.
The template string has inconsistent spacing. Line 607 has two spaces before ${exportsBlock}, while line 606 has one space before ${defaultBlock}. This results in extra whitespace in the generated declaration files. Consider using consistent single-space indentation or restructuring the template to avoid manual spacing.
| export default defineConfig({ | ||
| testDir: 'test', | ||
| timeout: 20_000, | ||
| timeout: 5_000, |
There was a problem hiding this comment.
The test timeout has been reduced from 20,000ms to 5,000ms. This is a significant reduction that could cause existing tests to fail if they legitimately need more time. Ensure that all tests in the suite can consistently complete within this new 5-second timeout, especially in CI environments which may have slower performance.
| timeout: 5_000, | |
| timeout: isCI ? 20_000 : 5_000, |
| function splitQuery(query: string): string[] { | ||
| const trimmed = query.startsWith('?') ? query.slice(1) : query | ||
| if (!trimmed) return [] | ||
| return trimmed.split('&').filter(Boolean) | ||
| } | ||
|
|
||
| function isQueryFlag(entry: string, flag: string): boolean { | ||
| const [rawKey] = entry.split('=') | ||
| try { | ||
| return decodeURIComponent(rawKey) === flag | ||
| } catch { | ||
| return rawKey === flag | ||
| } | ||
| } | ||
|
|
||
| function hasQueryFlag(query: string, flag: string): boolean { | ||
| if (!query) return false | ||
| const entries = splitQuery(query) | ||
| if (entries.length === 0) return false | ||
| return entries.some(part => isQueryFlag(part, flag)) | ||
| } | ||
|
|
||
| function hasStableQuery(query: string): boolean { | ||
| return ( | ||
| hasQueryFlag(query, KNIGHTED_CSS_QUERY_FLAG) && hasQueryFlag(query, STABLE_QUERY_FLAG) | ||
| ) | ||
| } | ||
|
|
||
| function hasNamedOnlyQueryFlag(query: string): boolean { | ||
| return hasQueryFlag(query, 'named-only') || hasQueryFlag(query, 'no-default') | ||
| } |
There was a problem hiding this comment.
The functions splitQuery, isQueryFlag, hasQueryFlag, and hasNamedOnlyQueryFlag are duplicated between this file and loaderInternals.ts. This code duplication could lead to inconsistencies if one implementation is updated but not the other. Consider extracting these shared utilities to a common module that both files can import.
| } | ||
| } | ||
| const stableSpecifierRegex = | ||
| /(?:import\s+(?:[^'"`]+?\s+from\s+)?|export\s+[^'"`]+?\s+from\s+|import\s*\(\s*|require\()(['"])([^'"`]+?\?[^'"`]*knighted-css[^'"`]*stable[^'"`]*)\1/g |
There was a problem hiding this comment.
The regex pattern on line 388 attempts to match stable query imports but has a potential issue: it looks for "knighted-css" followed by "stable" anywhere in the query string, without requiring the "stable" flag to be properly delimited by ampersands. This could match unintended patterns like "?knighted-css&unstable" or "?knighted-css-stable". Consider using a more precise pattern that validates query parameter boundaries, similar to how the hasStableQuery function validates individual query flags.
| /(?:import\s+(?:[^'"`]+?\s+from\s+)?|export\s+[^'"`]+?\s+from\s+|import\s*\(\s*|require\()(['"])([^'"`]+?\?[^'"`]*knighted-css[^'"`]*stable[^'"`]*)\1/g | |
| /(?:import\s+(?:[^'"`]+?\s+from\s+)?|export\s+[^'"`]+?\s+from\s+|import\s*\(\s*|require\()(['"])([^'"`]+?\?(?=[^'"`]*[?&]knighted-css(?:&|$))(?=[^'"`]*[?&]stable(?:&|$))[^'"`]*)\1/g |
| if (isIdentifier(binding)) { | ||
| return [binding.name] | ||
| } | ||
| return [] |
There was a problem hiding this comment.
The collectBindingNames function only handles Identifier bindings and returns an empty array for other binding patterns like destructuring. This means that named exports using destructuring patterns (e.g., export const { a, b } = obj) will not be tracked in the exports array. Consider adding support for ObjectPattern and ArrayPattern binding types to ensure complete export tracking for the stable query type generation feature.
| return [] | |
| const names: string[] = [] | |
| function visit(node: unknown): void { | |
| if (!node) return | |
| if (isIdentifier(node as BindingPatternLike)) { | |
| names.push((node as { name: string }).name) | |
| return | |
| } | |
| if (Array.isArray(node)) { | |
| for (const element of node) { | |
| visit(element) | |
| } | |
| return | |
| } | |
| if (typeof node !== 'object') { | |
| return | |
| } | |
| if (!('type' in (node as { type?: unknown }))) { | |
| return | |
| } | |
| const n = node as { type: string; [key: string]: unknown } | |
| switch (n.type) { | |
| case 'ObjectPattern': { | |
| // ESTree/oxc: properties: Array<...>, optional rest element | |
| visit(n.properties) | |
| // Some ASTs use "rest", others may use "restElement" or similar; handle "rest" if present. | |
| if ('rest' in n) { | |
| visit(n.rest) | |
| } | |
| return | |
| } | |
| case 'ArrayPattern': { | |
| // elements: Array<Pattern | null> | |
| visit(n.elements) | |
| return | |
| } | |
| case 'Property': | |
| case 'AssignmentProperty': { | |
| // Visit value; fall back to key if needed | |
| if ('value' in n && n.value != null) { | |
| visit(n.value) | |
| } else if ('key' in n) { | |
| visit(n.key) | |
| } | |
| return | |
| } | |
| case 'RestElement': { | |
| if ('argument' in n) { | |
| visit(n.argument) | |
| } | |
| return | |
| } | |
| case 'AssignmentPattern': { | |
| if ('left' in n) { | |
| visit(n.left) | |
| } | |
| return | |
| } | |
| default: | |
| return | |
| } | |
| } | |
| visit(binding) | |
| return names |
| `${resourcePath}?knighted-css&stable`, | ||
| '', | ||
| {}, |
There was a problem hiding this comment.
Superfluous arguments passed to function pitch.
|
Closing in favor of #57 |
No description provided.