Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/x-chat/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@
"node": ">=14.0.0"
},
"imports": {
"#formatErrorMessage": "@mui/x-internals/formatErrorMessage"
"#formatErrorMessage": "@mui/x-internals/formatErrorMessage",
"#remend": {
"import": "remend",
"default": "./src/internals/remendUnavailable.ts"
}
}
}
14 changes: 14 additions & 0 deletions packages/x-chat/src/internals/remendUnavailable.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* CommonJS resolution target for the `#remend` subpath import (see this package's
* `imports` field, which points ESM at the real `remend` package and CJS here).
*
* `remend` is ESM-only — its `exports` map declares no `require` condition — so the
* CJS build must not name it at all. A bundler that statically resolves a
* `require('remend')` in that output fails the build outright ("not exported under
* the conditions [...require...]"), which is why the specifier cannot simply be
* inlined for every format.
*
* Exporting no repair function is the signal: `loadRemend` sees a non-function and
* degrades to `fallbackRepair`, exactly as it does when `remend` is absent.
*/
export default undefined;
30 changes: 30 additions & 0 deletions packages/x-chat/src/internals/streamingMarkdownRepair.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,34 @@ describe('streamingMarkdownRepair', () => {
expect(repair('x')).to.equal('x-2');
});
});

// Regression coverage for the `remend` specifier resolving in bundled apps.
// Importing it under a specifier a bundler can't statically resolve left the upgrade
// as dead code in every browser bundle and — where the bundler wraps dynamic imports
// in a preload helper — dispatched a global load-error event on each render.
// See https://github.com/mui/mui-x/issues/23160.
describe('remend specifier resolution', () => {
it('resolves the real remend through the default importer', async () => {
// No injected importer: exercises `import('#remend')` for real, so a specifier
// that stopped resolving would fail here rather than silently degrade.
const repair = await loadRemend();

expect(repair).not.to.equal(fallbackRepair);
// remend completes the unterminated inline marker; fallbackRepair never would.
expect(repair('a **bold')).to.equal('a **bold**');
});

it('degrades to fallbackRepair on the CommonJS `#remend` stub', async () => {
// What `require('#remend')` resolves to in the CJS build, where the ESM-only
// `remend` cannot be named at all.
const stub = await import('./remendUnavailable');
const repair = await loadRemend(() => Promise.resolve(stub));

expect(repair).to.equal(fallbackRepair);
});

// The packaging side of this — that the specifier stays statically analyzable and
// that `#remend` maps per module format — reads the source tree off disk, so it
// lives in `src/tests/packagingGuard/remendSpecifier.test.ts` (Node-only).
});
});
33 changes: 17 additions & 16 deletions packages/x-chat/src/internals/streamingMarkdownRepair.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,29 +15,30 @@ export type RepairMarkdown = (text: string) => string;
*/
export const fallbackRepair: RepairMarkdown = normalizeMarkdownForRender;

// `remend` is an ESM-only package and a declared dependency of this package. The
// build downlevels dynamic `import()` to `require()` in the CJS output, and a static
// `require()` of an ES module throws `ERR_REQUIRE_ESM`. Reading the specifier from a
// variable (plus the `@vite-ignore`/`webpackIgnore` hints) keeps that `require`/
// `import` dynamic and unanalyzable, so it stays a genuine runtime import that fails
// gracefully at call time instead of at build time. The `.catch` in `loadRemend`
// then degrades to `fallbackRepair` — so a runtime that can't resolve the specifier
// (or a CJS `require()` of the ESM module) costs nothing beyond the failed attempt.
const REMEND_SPECIFIER = 'remend';
// `remend` is an ESM-only package and a declared dependency of this package, so the
// specifier it is imported under has to differ per output format. `#remend` is a
// subpath import (see this package's `imports` field) that resolves to the real
// `remend` package under the `import` condition and to a stub under `require`.
//
// The literal specifier matters: it has to stay statically analyzable so a consumer's
// bundler resolves and bundles `remend` like any other dependency. Reading it from a
// variable instead leaves an unresolvable bare `import('remend')` in the browser
// bundle, which can never resolve at runtime (no import map) — making the upgrade dead
// code, and, under bundlers that wrap dynamic imports in a preload helper, dispatching
// a global load-error event on every render.
function defaultRemendImporter(): Promise<unknown> {
// eslint-disable-next-line jsdoc/no-bad-blocks -- bundler hint comments, not JSDoc
return import(/* @vite-ignore */ /* webpackIgnore: true */ REMEND_SPECIFIER);
return import('#remend');
}

let remendPromise: Promise<RepairMarkdown> | undefined;

/**
* Lazily loads `remend` and returns a repair function. If the import rejects — e.g.
* the CJS build downlevelled it to a `require()` of the ESM-only module, or a
* consumer's bundler can't resolve the runtime specifier — it transparently degrades
* to {@link fallbackRepair}. Cached after the first call.
* Lazily loads `remend` and returns a repair function. When it can't be loaded — the
* CJS build resolves `#remend` to a stub, and a consumer may have deduped or blocked
* the dependency — it transparently degrades to {@link fallbackRepair}. Cached after
* the first call.
*
* @param importer Injectable for tests; defaults to `() => import('remend')`.
* @param importer Injectable for tests; defaults to `() => import('#remend')`.
*/
export function loadRemend(
importer: () => Promise<unknown> = defaultRemendImporter,
Expand Down
128 changes: 128 additions & 0 deletions packages/x-chat/src/tests/packagingGuard/remendEsmBundle.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
import fs from 'node:fs';
import path from 'node:path';
import { fileURLToPath, pathToFileURL } from 'node:url';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
import { build } from 'vite';

// Node-only (runs a real bundler and imports its output), so the browser project
// excludes this directory — see `vitest.config.browser.mts`.
//
// This is the coverage the unit tests structurally cannot provide. #23160 was invisible
// to every in-process test: `loadRemend` resolved `remend` fine under Node and under the
// Vitest dev server, while the *bundled* output shipped to users contained a bare
// `import('remend')` that could never resolve in a browser. The upgrade was dead code
// everywhere it mattered, and nothing failed.
//
// So bundle the loader the way a consumer's bundler does, then run the bundle and check
// the markdown actually comes back repaired.
const packageRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../../..');
// Under `node_modules` so the scratch artifacts are git-ignored. The generated entry is
// kept out of `outDir` so that reading the build output back never picks it up.
const workDir = path.join(packageRoot, 'node_modules', '.tmp-esm-bundle-test');
const outDir = path.join(workDir, 'out');

/** The subset of Rollup's chunk record this test asserts on. */
interface EmittedChunk {
type: string;
fileName: string;
isEntry: boolean;
/** Static imports, as resolved by the bundler: emitted file names, or bare if external. */
imports: string[];
/** Dynamic imports, same resolution rules. */
dynamicImports: string[];
}

let chunks: EmittedChunk[];
let loadRemend: () => Promise<(text: string) => string>;
let fallbackRepair: (text: string) => string;

describe('remend in an ESM bundle', () => {
beforeAll(async () => {
fs.rmSync(workDir, { recursive: true, force: true });
fs.mkdirSync(outDir, { recursive: true });

const entry = path.join(workDir, 'entry.mjs');
const loader = path.join(packageRoot, 'src/internals/streamingMarkdownRepair.ts');
fs.writeFileSync(
entry,
`export { loadRemend, fallbackRepair } from ${JSON.stringify(loader)};\n`,
);

const result = await build({
root: packageRoot,
logLevel: 'silent',
build: {
outDir,
emptyOutDir: false,
minify: false,
target: 'esnext',
lib: { entry, formats: ['es'], fileName: 'bundle' },
// Bundle everything. Leaving anything external means the emitted module has to
// resolve bare specifiers from a temp directory at import time, which is exactly
// the fragility being tested for — and it does not survive CI's install layout.
// Nothing here is slow: the hook (and with it React) tree-shakes away, since the
// entry only re-exports the loader.
rollupOptions: { external: [] },
},
});

// Assert on the bundler's own parsed import records rather than on the emitted text.
// Scanning the output with a regex is unsound: it bundles third-party source, and a
// plain string literal in there (React 18's warning templates, for one) reads exactly
// like an import specifier.
const output = (Array.isArray(result) ? result[0] : result) as unknown as {
output: EmittedChunk[];
};
chunks = output.output.filter((item) => item.type === 'chunk');

const mod = await import(pathToFileURL(path.join(outDir, 'bundle.mjs')).href);
loadRemend = mod.loadRemend;
fallbackRepair = mod.fallbackRepair;
}, 120_000);

afterAll(() => {
fs.rmSync(workDir, { recursive: true, force: true });
});

it('repairs markdown with the real remend, not the fallback', async () => {
const repair = await loadRemend();

// The whole point of the dependency: complete an unterminated inline marker.
// `fallbackRepair` only closes code fences, so it would return this unchanged.
expect(repair('a **bold')).to.equal('a **bold**');
expect(repair).not.to.equal(fallbackRepair);
expect(fallbackRepair('a **bold')).to.equal('a **bold');
});

// Note: the assertion above would not on its own have caught #23160, because Node
// resolves a bare `import('remend')` from `node_modules` even when the bundler left one
// behind. A browser cannot, which is why the bug shipped. The runtime catch lives in
// the Chromium run of `streamingMarkdownRepair.test.ts`; the checks below are what make
// this file a regression guard.

it('emits remend as a chunk instead of leaving a bare specifier', () => {
const emittedNames = chunks.map((chunk) => chunk.fileName);
const entryChunk = chunks.find((chunk) => chunk.isEntry);
const dynamicImports = entryChunk?.dynamicImports ?? [];

// Before the fix the specifier was unanalyzable, so the bundler recorded no dynamic
// import at all and emitted nothing for it — the browser was left to resolve `remend`
// on its own, which it cannot do.
expect(dynamicImports).not.to.deep.equal([]);
// Every dynamic import resolves to something the bundler emitted itself.
dynamicImports.forEach((id) => expect(emittedNames).to.contain(id));
// remend lands in its own chunk alongside the entry.
expect(chunks.length).to.be.greaterThan(1);
});

it('emits a self-contained bundle', () => {
// Guards the test itself: if anything were left external, importing the output above
// would prove nothing, because Node could paper over it from a nearby `node_modules`.
const emittedNames = chunks.map((chunk) => chunk.fileName);
const external = chunks
.flatMap((chunk) => [...chunk.imports, ...chunk.dynamicImports])
.filter((id) => !emittedNames.includes(id));

expect(external).to.deep.equal([]);
});
});
45 changes: 45 additions & 0 deletions packages/x-chat/src/tests/packagingGuard/remendSpecifier.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import fs from 'node:fs';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
import { describe, expect, it } from 'vitest';

// Node-only: reads the source tree and package.json off disk, so the browser project
// excludes this directory (see `vitest.config.browser.mts`). The runtime behaviour of
// the loader is covered in `src/internals/streamingMarkdownRepair.test.ts`, which stays
// browser-compatible.
//
// `remend` is ESM-only, so the specifier it is imported under has to differ per output
// format. Getting that wrong is invisible to a unit test but breaks consumers: a
// specifier a bundler can't statically resolve leaves an unresolvable bare import in the
// browser bundle — dead code, plus a global load-error event under bundlers that wrap
// dynamic imports in a preload helper. Naming `remend` directly in the CJS output fails
// the build instead. See https://github.com/mui/mui-x/issues/23160.
const packageRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../../..');

describe('remend specifier packaging', () => {
it('imports a statically analyzable specifier', () => {
const source = fs.readFileSync(
path.join(packageRoot, 'src/internals/streamingMarkdownRepair.ts'),
'utf8',
);

expect(source).to.contain("import('#remend')");
// A specifier read from a variable, or hidden behind an ignore hint, is
// unanalyzable: bundlers leave a bare specifier that can never resolve in a
// browser instead of bundling the dependency.
expect(source).not.to.match(/import\(\s*(\/\*[^*]*\*\/\s*)*[A-Za-z_$]/);
});

it('maps `#remend` per module format in package.json', () => {
const packageJson = JSON.parse(fs.readFileSync(path.join(packageRoot, 'package.json'), 'utf8'));
const remendImport = packageJson.imports['#remend'];

// ESM gets the real package, so bundlers resolve and bundle it.
expect(remendImport.import).to.equal('remend');
// Every other condition (CJS) resolves to a file inside this package rather than
// to `remend`, which declares no `require` export — a bundler resolving that in
// the CJS output fails the build outright.
expect(remendImport.default).to.match(/^\.\//);
expect(packageJson.dependencies.remend).to.be.a('string');
});
});
10 changes: 10 additions & 0 deletions packages/x-chat/vitest.config.browser.mts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ import { getTestName } from '../../scripts/getTestName.mts';
export default mergeConfig(
sharedConfig,
defineConfig({
// `remend` is only reached through the lazy `import('#remend')` in
// `streamingMarkdownRepair.ts`, so Vite's dependency scanner does not see it up
// front. It gets discovered the first time a test actually loads it, and the
// resulting re-optimization reloads the page mid-run, which drops every in-flight
// suite in this project. Pre-bundling it keeps the run stable.
optimizeDeps: {
include: ['remend'],
},
test: {
name: getTestName(import.meta.url),
browser: {
Expand All @@ -17,6 +25,8 @@ export default mergeConfig(
'**/dist/**',
// Docs-correctness guard uses Node-only modules (fs, typescript)
'**/docsCorrectnessGuard/**',
// Packaging guard reads the source tree and package.json with Node-only fs
'**/packagingGuard/**',
],
},
}),
Expand Down
Loading