-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[chat] Fix global preload error from the remend import
#23263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
de36528
e12a218
862a26f
2dc4820
4c4e35f
b066522
9666099
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,11 @@ | ||
| import fs from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { describe, expect, it, vi, beforeEach } from 'vitest'; | ||
| import { fallbackRepair, loadRemend, resetRemendCache } from './streamingMarkdownRepair'; | ||
|
|
||
| const packageRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for looking, but I don't think this one holds — it's one The test file is at The reads resolve to Note the code has since moved to |
||
|
|
||
| describe('streamingMarkdownRepair', () => { | ||
| beforeEach(() => { | ||
| resetRemendCache(); | ||
|
|
@@ -65,4 +70,58 @@ 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); | ||
| }); | ||
|
|
||
| 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'); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The x-chat browser configuration includes this test file, so
pnpm test:browser --project "x-chat" --runwill load these top-levelnode:fs,node:path, andnode:urlimports in Chromium. Vite externalizes Node built-ins for browser compatibility, and the top-levelpath.resolve(...)or laterfs.readFileSync(...)will fail the suite before these assertions can run. Move the filesystem-based regression checks into a separately excluded Node-only test file, as is already done fordocsCorrectnessGuard, while leaving the runtime tests browser-compatible.AGENTS.md reference: AGENTS.md:L40-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — this was the cause of the
test_browser/test_browser_react_18failures, and you're right that the file never even got to the assertions: it failed to collect in Chromium (streamingMarkdownRepair.test.ts (0 test)), while the other 284 browser tests passed. I'd only runtest:unitlocally, which is why it slipped through.Fixed in e12a218 exactly as suggested, following the
docsCorrectnessGuardprecedent:src/tests/packagingGuard/remendSpecifier.test.ts**/packagingGuard/**to the exclude list invitest.config.browser.mtsstreamingMarkdownRepair.test.tsKeeping the runtime tests browser-compatible turned out to be worth more than just unblocking CI — they now assert in Chromium that
#remendresolves and thatremendactually loads, which is precisely the behaviour #23160 was about. That file went from 0 collected tests to 11.Verified: browser 27 files / 295 tests passing, unit 940 passing / 33 skipped. I also re-ran the mutation check (restoring the old variable specifier) to confirm the guard still fails from its new location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction to my reply above: I attributed the
test_browserfailures to this issue, and that was wrong. I inferred it from timing instead of reading the CI log. Pulling the log showed the original run had failed identically — all 27 x-chat suites, samesetupVitesterror — both before and after this fix. Your finding was a real defect (the file did fail to collect in Chromium locally) but it was not what CI was red on.The actual cause was a side effect of the fix working. Now that
import('#remend')genuinely resolves, the browser suite loadsremendfor real, and Vite's scanner can't see it up front behind a lazy subpath import:The mid-run re-optimization reloads the page and drops every in-flight suite in the project. It passed locally only because my optimizer cache was warm — I reproduced it by clearing
node_modules/.vite. Fixed in 862a26f by addingremendtooptimizeDeps.include, as that warning recommends.test_browsernow gets through the whole x-chat project; the only remaining failure is an unrelated flakyDataGridProdata-source test (expected 11 to equal 12, retried 4x).