From 445edbaa5f461687d26c876285c09834d4e193a9 Mon Sep 17 00:00:00 2001 From: Okiki Ojo Date: Fri, 30 Jan 2026 22:18:19 -0500 Subject: [PATCH] chore: fix cdn resolution for subpath Signed-off-by: Okiki Ojo --- core/plugins/alias.ts | 30 ++++++++++++------- core/plugins/cdn.ts | 56 ++++++++++++++++++++++++++++++------ core/plugins/http.ts | 4 ++- core/utils/cdn-resolution.ts | 37 +++++++++++++++++++++--- utils/npm-search.ts | 6 ++++ utils/path.ts | 13 ++++++++- 6 files changed, 121 insertions(+), 25 deletions(-) diff --git a/core/plugins/alias.ts b/core/plugins/alias.ts index 9a54eb8..2edb1f1 100644 --- a/core/plugins/alias.ts +++ b/core/plugins/alias.ts @@ -1,18 +1,23 @@ import type { ESBUILD, LocalState } from "../types.ts"; -import type { Context } from "../context/context.ts"; -import { fromContext } from "../context/context.ts"; +import { Context, fromContext, withContext } from "../context/context.ts"; import { EXTERNALS_NAMESPACE } from "./external.ts"; import { HttpResolution } from "./http.ts"; import { parsePackageName } from "@bundle/utils/parse-package-name"; +import { looksLikeJSRSpec } from "@bundle/utils/jsr-spec"; import { isBareImport } from "@bundle/utils/path"; + import { getCDNUrl } from "../utils/cdn-format.ts"; /** Alias Plugin Namespace */ export const ALIAS_NAMESPACE = "alias-globals"; +export interface AliasResolutionState extends LocalState { + build: ESBUILD.PluginBuild +} + /** * Checks if a package has an alias * @@ -20,7 +25,11 @@ export const ALIAS_NAMESPACE = "alias-globals"; * @param aliases An object with package as the key and the package alias as the value, e.g. { "fs": "memfs" } */ export function isAlias(id: string, aliases = {}) { - if (!isBareImport(id)) return false; + if ( + !isBareImport(id) && + !/^#/.test(id) && + !looksLikeJSRSpec(id) + ) return false; const aliasKeys = Object.keys(aliases); const path = id.replace(/^node\:/, ""); @@ -38,7 +47,7 @@ export function isAlias(id: string, aliases = {}) { * @param host The default host origin to use if an import doesn't already have one * @param logger Console log */ -export function AliasResolution(StateContext: Context>) { +export function AliasResolution(StateContext: Context>) { const LocalConfig = fromContext("config", StateContext)!; const { alias: aliases = {} } = LocalConfig; @@ -51,10 +60,9 @@ export function AliasResolution(StateContext: Context>) { const aliasPath = aliases[pkgDetails.name]; if (aliasPath) { - return HttpResolution(StateContext)({ - ...args, + return HttpResolution(StateContext)(Object.assign({}, args, { path: aliasPath - }); + })); } } }; @@ -75,13 +83,15 @@ export function AliasPlugin(StateContext: Context>): ESBUILD.Pl return { name: ALIAS_NAMESPACE, setup(build) { + const ctx = withContext({ build: Context.opaque(build) }, StateContext); + // Intercept import paths starting with "http:" and "https:" so // esbuild doesn't attempt to map them to a file system location. // Tag them with the "http-url" namespace to associate them with // this plugin. build.onResolve({ filter: /^node\:.*/ }, (args) => { if (isAlias(args.path, aliases)) - return AliasResolution(StateContext)(args); + return AliasResolution(ctx)(args); if (!polyfill) { return { @@ -97,8 +107,8 @@ export function AliasPlugin(StateContext: Context>): ESBUILD.Pl // files will be in the "http-url" namespace. Make sure to keep // the newly resolved URL in the "http-url" namespace so imports // inside it will also be resolved as URLs recursively. - build.onResolve({ filter: /.*/ }, AliasResolution(StateContext)); - build.onResolve({ filter: /.*/, namespace: ALIAS_NAMESPACE }, AliasResolution(StateContext)); + build.onResolve({ filter: /.*/ }, AliasResolution(ctx)); + build.onResolve({ filter: /.*/, namespace: ALIAS_NAMESPACE }, AliasResolution(ctx)); }, }; }; \ No newline at end of file diff --git a/core/plugins/cdn.ts b/core/plugins/cdn.ts index da80d53..c9d4247 100644 --- a/core/plugins/cdn.ts +++ b/core/plugins/cdn.ts @@ -180,17 +180,29 @@ export function CdnResolution(StateContext: Context>) { // ======================================================================== // Handle subpath imports (#internal/...) // https://nodejs.org/api/packages.html#subpath-imports + // + // IMPORTANT: Private imports MUST be resolved against the IMPORTER's manifest, + // not the root manifest. The vfile package imports #minpath, so we need + // vfile's package.json (passed via pluginData.manifest), not the entry point's. // ======================================================================== + console.log({ argPath}) if (/^#/.test(argPath)) { - // NEW: Use resolveModern for subpath imports - const result = resolveModern(initialManifest, argPath, conditions); + // Use importer's manifest if available, fall back to initial manifest + const importerManifest: Partial = + args.pluginData?.manifest ?? initialManifest; + + const manifestName = importerManifest.name ?? initialManifest.name ?? "unknown"; + const manifestVersion = importerManifest.version ?? initialManifest.version ?? "latest"; + + // Try to resolve the subpath import + const result = resolveModern(importerManifest, argPath, conditions); if (result.success && result.path) { - argPath = join(initialManifest.name + "@" + initialManifest.version, result.path); + argPath = join(`${manifestName}@${manifestVersion}`, result.path); } else if (!result.success && !conditions.require) { // Compatibility fallback: try require conditions - const fallback = resolveModern(initialManifest, argPath, { + const fallback = resolveModern(importerManifest, argPath, { ...conditions, require: true, conditions: ["require", ...conditions.conditions], @@ -198,6 +210,16 @@ export function CdnResolution(StateContext: Context>) { if (fallback.success && fallback.path) { argPath = join(initialManifest.name + "@" + initialManifest.version, fallback.path); + } else { + // CRITICAL: Private import resolution failed + // Return error immediately - do NOT fall through to bare import handling + // as this would incorrectly try to resolve #minpath as an npm package + return { + errors: [{ + text: `Failed to resolve private import "${argPath}"`, + detail: `The package "${manifestName}" does not define this import in its "imports" field, or the import conditions [${conditions.conditions.join(", ")}] don't match. This is a Node.js subpath import that must be defined in package.json.`, + }], + }; } } } @@ -492,15 +514,31 @@ export function CdnResolution(StateContext: Context>) { }); if (resolutionResult.excluded && conditions.browser) { - // Module is excluded for browser (e.g., browser: false) + // Generate accurate error message based on exclusion reason + const reason = (resolutionResult as { exclusionReason?: string }).exclusionReason; + + let text: string; + let detail: string; + + if (reason === "browser" || reason === "browser-remapping") { + text = `Package "${effectiveName}" is excluded for browser environment`; + detail = "The package's browser field explicitly excludes this module."; + } else if (reason === "no-entry-point") { + text = `Could not find entry point for package "${effectiveName}"`; + detail = `The package.json does not define 'main', 'module', 'exports', or other entry point fields. Subpath: "${combinedSubpath || "."}".`; + } else { + text = `Package resolution failed for "${effectiveName}"`; + detail = resolutionResult.error?.message ?? "Unknown resolution error"; + } + return { - errors: [{ - text: `Package "${effectiveName}" is excluded for browser environment`, - detail: "The package's browser field explicitly excludes this module.", - }], + errors: [{ text, detail }], }; } + // If we're not building for browser, don't treat this as a hard exclusion. + // Prefer to continue and let normal "no entry point" errors surface if needed. + // (With the resolveLegacy + resolvePackageEntry fixes, this path should rarely trigger.) if (resolutionResult.error) { dispatchEvent(LOGGER_WARN, `Resolution error for ${effectiveName}: ${resolutionResult.error.message}`); } diff --git a/core/plugins/http.ts b/core/plugins/http.ts index 70a388a..5546c14 100644 --- a/core/plugins/http.ts +++ b/core/plugins/http.ts @@ -39,6 +39,7 @@ import { setFile } from "../utils/filesystem.ts"; import { isBareImport, isAbsolute } from "@bundle/utils/path"; import { toURLPath, urlJoin } from "@bundle/utils/url"; +import { looksLikeJSRSpec } from "@bundle/utils/jsr-spec"; /** HTTP Plugin Namespace */ export const HTTP_NAMESPACE = "http-url"; @@ -297,7 +298,8 @@ export function HttpResolution(StateContext: Context>) const origin = NPM_CDN ? pathOrigin : host; // Bare import (e.g., "lodash") → delegate to CDN resolution - if (isBareImport(argPath)) { + // Also handle private imports (#internal) and JSR spec imports + if (/^#/.test(argPath) || isBareImport(argPath) || looksLikeJSRSpec(argPath)) { const ctx = withContext({ origin, build: Context.opaque(build) }, StateContext); return await CdnResolution(ctx)(args); } diff --git a/core/utils/cdn-resolution.ts b/core/utils/cdn-resolution.ts index a551c78..4058f94 100644 --- a/core/utils/cdn-resolution.ts +++ b/core/utils/cdn-resolution.ts @@ -30,7 +30,7 @@ import type { PackageJson, FullPackageVersion } from "@bundle/utils/types"; import type { SideEffectsMatchers } from "./side-effects.ts"; import type { ResolverConditions as BaseResolverConditions } from "@bundle/utils/resolve-conditions"; -import { resolve, legacy, imports } from "@bundle/utils/resolve-exports-imports"; +import { resolve, legacy } from "@bundle/utils/resolve-exports-imports"; import { computeEsbuildSideEffects } from "./side-effects.ts"; import { dispatchEvent, LOGGER_WARN } from "../configs/events.ts"; @@ -53,6 +53,9 @@ export interface ModernResolutionResult { error?: Error; } +/** Reasons for exclusion via browser field */ +export type ExclusionReason = "browser" | "browser-remapping" | "no-entry-point"; + /** Result from legacy field resolution */ export interface LegacyResolutionResult { /** Resolved entry point (from main/module, NOT browser object keys) */ @@ -61,6 +64,8 @@ export interface LegacyResolutionResult { browserRemappings: BrowserRemappings | null; /** Whether module is excluded (browser: false) */ excluded: boolean; + /** Why the module was excluded - helps generate accurate error messages */ + exclusionReason?: ExclusionReason; error?: Error; } @@ -227,6 +232,7 @@ export function resolveLegacy( // Case: browser field is `false` - module excluded if (!withBrowser) { result.excluded = true; + result.exclusionReason = "browser"; return result; } @@ -252,6 +258,7 @@ export function resolveLegacy( if (allFalse) { // Package has no browser support result.excluded = true; + result.exclusionReason = "browser"; return result; } @@ -271,7 +278,10 @@ export function resolveLegacy( }); if (!entryPoint) { + // Note: This is NOT browser exclusion - it's "no entry point found" + // The error message should reflect this distinction result.excluded = true; + result.exclusionReason = "no-entry-point"; return result; } @@ -401,9 +411,28 @@ export function resolvePackageEntry(options: PackageResolutionOptions): PackageR // Check if excluded for browser if (conditions.browser && legacyResult.excluded) { - result.excluded = true; - result.error = new Error("Module excluded by browser field"); - return result; + const reason = legacyResult.exclusionReason; + + // Browser exclusion only matters when targeting browser + // When platform is 'node', browser: false doesn't exclude the module + if (reason === "browser" && !conditions.browser) { + // Platform is node, browser exclusion doesn't apply + // Continue resolution - module might have valid Node.js entry + dispatchEvent(LOGGER_WARN, + `Package excluded for browser but platform is not browser, continuing resolution`); + } else if (reason === "no-entry-point") { + // No entry point is an error regardless of platform + result.excluded = true; + result.error = new Error("No entry point found in package.json"); + (result as { exclusionReason?: string }).exclusionReason = reason; + return result; + } else { + // Browser exclusion when targeting browser + result.excluded = true; + result.error = new Error("Module excluded by browser field"); + (result as { exclusionReason?: string }).exclusionReason = reason; + return result; + } } if (legacyResult.entryPoint) { diff --git a/utils/npm-search.ts b/utils/npm-search.ts index 5ca6aa1..69d2024 100644 --- a/utils/npm-search.ts +++ b/utils/npm-search.ts @@ -64,6 +64,12 @@ export const DEFAULT_REGISTRY = "https://registry.npmjs.com"; * escapePackageName("react") // "react" * escapePackageName("@types/node") // "@types%2fnode" * escapePackageName("@tanstack/react-query") // "@tanstack%2freact-query" + * + * @example + * escapePackageName("@mdx-js/mdx") -> "@mdx-js%2fmdx" + * + * @example + * escapePackageName("#minpath") -> "%23minpath" */ export function escapePackageName(name: string): string { return name.replace("/", "%2f"); diff --git a/utils/path.ts b/utils/path.ts index 5443359..1671af1 100644 --- a/utils/path.ts +++ b/utils/path.ts @@ -5,8 +5,19 @@ export * as posix from "@std/path/posix"; export * as windows from "@std/path/windows"; /** - * An import counts as a bare import if it's neither a relative import of an absolute import + * An import counts as a bare import if it's neither: + * - A relative import (./foo, ../bar) + * - An absolute import (/foo, C:\foo) + * - A data: URL + * - A private/subpath import (#internal) + * + * Private imports (#...) are Node.js subpath imports that must be resolved + * against the importer's package.json "imports" field, not treated as npm packages. + * @see https://nodejs.org/api/packages.html#subpath-imports */ export const isBareImport = (importStr: string) => { + // Private imports are NOT bare imports - they have special resolution rules + if (/^#/.test(importStr)) return false; + return /^(?!\.).*/.test(importStr) && !importStr.startsWith("data:") && !isAbsolute(importStr); }; -- 2.54.0