Skip to content

Commit 846e909

Browse files
deyaaeldeenCopilot
andcommitted
feat(warp trace): add explicit warnings for missing #platform imports
Detects when a platform-specific file (e.g., -browser.mts) directly imports a non-platform file that has a platform variant available. Shows warning: ⚠️ browser: src/indexPlatform-browser.mts imports: ./policies/StorageBrowserPolicyV2.js issue: "StorageBrowserPolicyV2.js" imported directly fix: Use #platform/... to get "StorageBrowserPolicyV2-browser.mts" This catches the exact bug that caused storage-common test failures where the browser entry point accidentally imported the Node version. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent e63c1d5 commit 846e909

1 file changed

Lines changed: 146 additions & 14 deletions

File tree

common/tools/warp/src/trace.ts

Lines changed: 146 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ export interface ImportNode {
5656
children: ImportNode[];
5757
/** Annotation for the resolved file (e.g., "(Node no-op)" or "(browser)") */
5858
annotation?: string;
59+
/** Warning if this import bypasses platform resolution */
60+
warning?: string;
5961
}
6062

6163
/** Import graph for a single target. */
@@ -66,12 +68,30 @@ export interface TargetImportGraph {
6668
entryPoints: ImportNode[];
6769
}
6870

71+
/** Warning about a potential platform import misconfiguration */
72+
export interface PlatformWarning {
73+
/** Type of warning */
74+
type: "missing-platform-import" | "wrong-variant";
75+
/** Target where the issue was found */
76+
target: string;
77+
/** File containing the problematic import */
78+
file: string;
79+
/** The import specifier */
80+
specifier: string;
81+
/** Human-readable message */
82+
message: string;
83+
/** Suggested fix */
84+
suggestion?: string;
85+
}
86+
6987
/** Result of tracing imports across all targets. */
7088
export interface TraceResult {
7189
/** Per-target import graphs */
7290
graphs: TargetImportGraph[];
7391
/** Cross-target comparison: specifiers that resolve differently */
7492
divergences: ImportDivergence[];
93+
/** Warnings about potential misconfigurations */
94+
warnings: PlatformWarning[];
7595
}
7696

7797
/** A specifier that resolves to different files across targets. */
@@ -96,17 +116,80 @@ export interface ImportDivergence {
96116
* - Files ending in `-native.mts` or `-native.ts` → "(react-native)"
97117
* - Platform entry files with "Node" in name → "(Node no-op)"
98118
*/
99-
function detectAnnotation(resolvedPath: string | undefined): string | undefined {
119+
function detectAnnotation(
120+
resolvedPath: string | undefined,
121+
targetName: string,
122+
): string | undefined {
100123
if (!resolvedPath) return "(unresolved)";
101124

102125
const basename = path.basename(resolvedPath);
126+
const filePlatform = detectFilePlatform(resolvedPath);
127+
128+
if (!filePlatform) return undefined;
103129

104-
if (basename.includes("-browser.")) return "(browser)";
105-
if (basename.includes("-native.")) return "(react-native)";
106-
if (basename.includes("-node.")) return "(Node)";
130+
// Check if the file's platform matches the current target
131+
const normalizedTarget = targetName.toLowerCase();
132+
const normalizedPlatform = filePlatform.toLowerCase();
107133

108-
// Detect Node no-op pattern: non-platform-suffixed file that has a browser variant
109-
// This is harder to detect without checking sibling files, so we leave it to heuristics
134+
if (normalizedTarget === normalizedPlatform) {
135+
return `(${filePlatform})`;
136+
}
137+
138+
// Cross-target import! Highlight it
139+
return `(${filePlatform} ⚠️ imported by ${targetName})`;
140+
}
141+
142+
/**
143+
* Detect the platform suffix from a file path.
144+
*/
145+
function detectFilePlatform(filePath: string): string | undefined {
146+
const basename = path.basename(filePath);
147+
148+
if (basename.includes("-browser.")) return "browser";
149+
if (basename.includes("-react-native.")) return "react-native";
150+
if (basename.includes("-native.")) return "react-native";
151+
if (basename.includes("-node.")) return "node";
152+
if (basename.includes("-workerd.")) return "workerd";
153+
154+
return undefined;
155+
}
156+
157+
/**
158+
* Check if a direct import should have used a #platform import instead.
159+
* Uses the package.json imports field to determine if a #platform equivalent exists.
160+
*/
161+
function checkMissingPlatformImport(
162+
specifier: string,
163+
resolvedPath: string | undefined,
164+
fromFile: string,
165+
targetName: string,
166+
importsMap: ImportsMap | undefined,
167+
conditions: ReadonlySet<string>,
168+
packageRoot: string,
169+
): PlatformWarning | undefined {
170+
if (specifier.startsWith("#") || !resolvedPath || !importsMap) return undefined;
171+
172+
const fromPlatform = detectFilePlatform(fromFile);
173+
if (!fromPlatform) return undefined;
174+
175+
const relPath = specifier.replace(/^\.\//, "").replace(/\.(js|ts|mts|mjs)$/, "");
176+
const platformSpecifier = `#platform/${relPath}`;
177+
const platformResolved = resolveSubpathImport(platformSpecifier, importsMap, conditions);
178+
if (!platformResolved) return undefined;
179+
180+
const platformAbsPath = path.resolve(packageRoot, platformResolved);
181+
if (!fs.existsSync(platformAbsPath)) return undefined;
182+
183+
if (platformAbsPath !== resolvedPath) {
184+
return {
185+
type: "missing-platform-import",
186+
target: targetName,
187+
file: fromFile,
188+
specifier,
189+
message: `imports "${path.relative(packageRoot, resolvedPath)}" but #platform resolves to "${path.relative(packageRoot, platformAbsPath)}"`,
190+
suggestion: `Use "${platformSpecifier}" to get the correct ${targetName} variant`,
191+
};
192+
}
110193

111194
return undefined;
112195
}
@@ -237,10 +320,30 @@ async function buildImportGraph(
237320
visited: Set<string>,
238321
depth: number,
239322
maxDepth: number,
323+
targetName: string,
324+
warnings: PlatformWarning[],
325+
fromFile?: string,
240326
): Promise<ImportNode> {
241327
const isPlatformImport = specifier.startsWith("#");
242328
const displayPath = resolvedPath ? path.relative(packageRoot, resolvedPath) : undefined;
243-
const annotation = detectAnnotation(resolvedPath);
329+
const annotation = detectAnnotation(resolvedPath, targetName);
330+
331+
let warning: string | undefined;
332+
if (fromFile && resolvedPath) {
333+
const platformWarning = checkMissingPlatformImport(
334+
specifier,
335+
resolvedPath,
336+
fromFile,
337+
targetName,
338+
importsMap,
339+
conditions,
340+
packageRoot,
341+
);
342+
if (platformWarning) {
343+
warnings.push(platformWarning);
344+
warning = `⚠️ ${platformWarning.suggestion}`;
345+
}
346+
}
244347

245348
const node: ImportNode = {
246349
specifier,
@@ -249,20 +352,18 @@ async function buildImportGraph(
249352
isPlatformImport,
250353
children: [],
251354
annotation,
355+
warning,
252356
};
253357

254-
// Stop if we've hit the depth limit or can't resolve the file
255358
if (depth >= maxDepth || !resolvedPath || visited.has(resolvedPath)) {
256359
return node;
257360
}
258361

259362
visited.add(resolvedPath);
260363

261-
// Only follow imports that are platform-relevant (# imports or lead to them)
262364
const imports = await extractImports(resolvedPath);
263365

264366
for (const importSpec of imports) {
265-
// Only trace `#`-prefixed imports and relative imports that might contain them
266367
if (importSpec.startsWith("#") || importSpec.startsWith(".")) {
267368
const childResolved = resolveSpecifier(
268369
importSpec,
@@ -281,10 +382,14 @@ async function buildImportGraph(
281382
visited,
282383
depth + 1,
283384
maxDepth,
385+
targetName,
386+
warnings,
387+
resolvedPath,
284388
);
285389

286-
// Only include if it's a platform import or has platform imports in children
287-
if (childNode.isPlatformImport || childNode.children.length > 0) {
390+
const isCrossTargetImport = childNode.annotation?.includes("⚠️") ?? false;
391+
const hasWarning = !!childNode.warning;
392+
if (childNode.isPlatformImport || childNode.children.length > 0 || isCrossTargetImport || hasWarning) {
288393
node.children.push(childNode);
289394
}
290395
}
@@ -426,7 +531,28 @@ export function formatTraceResult(result: TraceResult, packageRoot: string): str
426531
const useColor = process.stdout.isTTY ?? false;
427532
const lines: string[] = [];
428533

429-
// Section 1: Divergences (most important - show first)
534+
// Section 0: Warnings (most critical - show first if any)
535+
if (result.warnings.length > 0) {
536+
lines.push("╔══════════════════════════════════════════════════════════════╗");
537+
lines.push("║ ⚠️ WARNINGS - Potential Platform Import Issues ║");
538+
lines.push("╚══════════════════════════════════════════════════════════════╝");
539+
lines.push("");
540+
541+
for (const warning of result.warnings) {
542+
const relFile = path.relative(packageRoot, warning.file);
543+
const warningLine = useColor ? ANSI_RED + "⚠️ " + warning.target + ANSI_RESET : "⚠️ " + warning.target;
544+
lines.push(warningLine + ": " + relFile);
545+
lines.push(` imports: ${warning.specifier}`);
546+
lines.push(` issue: ${warning.message}`);
547+
if (warning.suggestion) {
548+
const fix = useColor ? ANSI_GREEN + warning.suggestion + ANSI_RESET : warning.suggestion;
549+
lines.push(` fix: ${fix}`);
550+
}
551+
lines.push("");
552+
}
553+
}
554+
555+
// Section 1: Divergences
430556
if (result.divergences.length > 0) {
431557
lines.push("╔══════════════════════════════════════════════════════════════╗");
432558
lines.push("║ Platform Import Divergences ║");
@@ -503,6 +629,7 @@ export async function traceImports(options: TraceOptions): Promise<TraceResult>
503629
const importsMap = await readPackageImports(packageRoot);
504630

505631
const graphs: TargetImportGraph[] = [];
632+
const allWarnings: PlatformWarning[] = [];
506633

507634
for (const target of config.targets) {
508635
const pc = parseTargetTsConfig(target, packageRoot);
@@ -532,6 +659,7 @@ export async function traceImports(options: TraceOptions): Promise<TraceResult>
532659

533660
const absSourcePath = path.resolve(packageRoot, sourcePath);
534661
const visited = new Set<string>();
662+
const warnings: PlatformWarning[] = [];
535663

536664
const rootNode = await buildImportGraph(
537665
sourcePath,
@@ -542,8 +670,12 @@ export async function traceImports(options: TraceOptions): Promise<TraceResult>
542670
visited,
543671
0,
544672
maxDepth,
673+
target.name,
674+
warnings,
545675
);
546676

677+
allWarnings.push(...warnings);
678+
547679
// Only include if there are platform imports somewhere
548680
if (rootNode.children.length > 0) {
549681
targetGraph.entryPoints.push(rootNode);
@@ -555,5 +687,5 @@ export async function traceImports(options: TraceOptions): Promise<TraceResult>
555687

556688
const divergences = findDivergences(graphs);
557689

558-
return { graphs, divergences };
690+
return { graphs, divergences, warnings: allWarnings };
559691
}

0 commit comments

Comments
 (0)