fix(plugins): make a module plugin actually re-evaluate on reload (#879) - #897
Merged
Merged
Conversation
A plugin reload silently did nothing for scriptType:"module" plugins. ES modules are evaluated ONCE PER URL PER DOCUMENT, so re-inserting a <script type="module"> whose src the module map has already seen fires `load` without re-running the body — and the loader then recorded the reload as applied. A no-op that reported success. THE ISSUE UNDERSTATES IT. #879 says "upgrades are fine — a new version yields a new URL". That is true of screen.js and FALSE of the plugin. I drove a real browser through install(1.0.0) -> upgrade(1.1.0) -> rollback(1.0.0), counting evaluations of src/main.js: ONE. Not three, not two. The upgrade re-runs the one-line screen.js shim at its new ?v= URL; the shim does `import './src/main.js'`; a relative specifier resolves against the base URL WITH THE QUERY DROPPED; that is the same URL as before; the module map hands back the already-evaluated v1.0.0 module. The plugin's own code never re-ran. Busting the entry point cannot fix this, whatever token you hang off it. So the token goes in the PATH: /api/plugins/<id>/g/<n>/screen.js. From there './src/main.js' resolves to /api/plugins/<id>/g/<n>/src/main.js — every relative import inherits it, at every depth, for free. No import-specifier rewriting (which could never see `import(expr)` anyway). Same browser drive after the fix: THREE evaluations. Keyed on the plugin ID, not id@version: EVERY re-load of a module plugin needs a fresh path, not just a rollback. First load keeps the stable ?v= URL, so the ETag/304 live-edit caching the R0 rails depend on is untouched. Classic-script plugins are not affected and never take a /g/ path. ━━━ A PATH REWRITE, NOT TWO MIRRORED ROUTES ━━━ Codex caught this, and it was right. The token shifts the BASE URL, so EVERYTHING the module graph resolves relatively moves with it — not only imports. `new URL('../assets/worklet.js', import.meta.url)` from /api/plugins/x/g/1/src/main.js resolves to /api/plugins/x/g/1/assets/worklet.js. Mirroring only screen.js and src/ would have fixed imports and 404'd every asset, worklet and wasm file the graph reaches — and would have broken again the next time someone added a plugin route. So the /g/<token> segment is STRIPPED BEFORE ROUTING. Every plugin route, present and future, works under the prefix with no extra wiring. The token is opaque and never joined into a filesystem path, so containment still rests entirely on the same safe_join. Codex then caught a [P3] in that: eagerly re-encoding raw_path with latin-1 raises UnicodeEncodeError on a valid plugin file like src/工具.js, 500ing a request the plain route serves fine. raw_path is informational and Starlette routes on scope["path"], so the mutation is simply gone — and leaving raw_path as the client sent it is more truthful for logs anyway. TESTS. tests/js/plugin_module_rollback.test.js (5) + 8 in test_plugin_src_route.py: identical bytes under the prefix, the whole graph one and two levels deep, ASSETS (the Codex [P2]), every plugin route, non-ASCII filenames (the [P3]), an opaque token, and containment asserted as PARITY with the un-prefixed route rather than a guessed 404 — `../screen.js` legitimately 200s on both, because the URL normalises before routing. All bite-tested: reverting the fix fails the rollback tests, disabling the rewrite fails the asset tests. Two harnesses re-anchored on `script.src = _pluginScriptUrl(` — the URL literal they keyed on now lives in the helper, further down the file, so their slice ran off the end. node 1045, pytest 2404, ESLint 0, Codex 0. Closes #879 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds generation-prefixed URLs for reloading ES-module plugins, rewrites those paths through existing plugin routes, and tests module graph resolution, containment, assets, Unicode filenames, and classic-script compatibility. ChangesPlugin module cache busting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PluginLoader
participant GenerationMiddleware
participant PluginRoute
PluginLoader->>GenerationMiddleware: Request generated screen.js URL
GenerationMiddleware->>PluginRoute: Rewrite path without generation segment
PluginRoute-->>PluginLoader: Return plugin resource
PluginLoader->>PluginLoader: Resolve relative imports under generated path
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #879.
A plugin reload silently did nothing for
scriptType: "module"plugins. ES modules are evaluated once per URL per document, so re-inserting a<script type="module">whosesrcthe module map has already seen firesloadwithout re-running the body — and the loader then recorded the reload as applied. A no-op that reported success.The issue understates it — upgrades were broken too
#879 says "upgrades are fine — a new version yields a new URL". That is true of
screen.jsand false of the plugin.I drove a real browser through install(1.0.0) → upgrade(1.1.0) → rollback(1.0.0), counting evaluations of
src/main.js:Not three. Not two. One.
The upgrade re-runs the one-line
screen.jsshim at its new?v=URL; the shim doesimport './src/main.js'; a relative specifier resolves against the base URL with the query string dropped; that's the same URL as before; the module map hands back the already-evaluated v1.0.0 module. The plugin's own code never re-ran.Busting the entry point cannot fix this, whatever token you hang off it.
The fix: the token goes in the PATH
/api/plugins/<id>/g/<n>/screen.js. From there'./src/main.js'resolves to/api/plugins/<id>/g/<n>/src/main.js— every relative import inherits the token, at every depth, for free. No import-specifier rewriting (which could never seeimport(expr)anyway).Same browser drive after the fix: 3 evaluations.
src/main.jsKeyed on the plugin id, not
id@version— every re-load of a module plugin needs a fresh path, not just a rollback. First load keeps the stable?v=URL, so the ETag/304 live-edit caching the R0 rails depend on is untouched. Classic-script plugins are unaffected and never take a/g/path.A path rewrite, not two mirrored routes
Codex caught this, and it was right. The token shifts the base URL, so everything the module graph resolves relatively moves with it — not only imports.
new URL('../assets/worklet.js', import.meta.url)from/api/plugins/x/g/1/src/main.jsresolves to/api/plugins/x/g/1/assets/worklet.js.Mirroring only
screen.jsandsrc/would have fixed imports and 404'd every asset, worklet and wasm file the graph reaches — and would have broken again the next time someone added a plugin route.So the
/g/<token>segment is stripped before routing. Every plugin route, present and future, works under the prefix with no extra wiring. The token is opaque and never joined into a filesystem path, so containment still rests entirely on the samesafe_join.Codex then caught a [P3] in that: eagerly re-encoding
raw_pathwith latin-1 raisesUnicodeEncodeErroron a valid plugin file likesrc/工具.js, 500ing a request the plain route serves fine.raw_pathis informational and Starlette routes onscope["path"], so the mutation is simply gone — and leavingraw_pathas the client sent it is more truthful for logs anyway.Tests
tests/js/plugin_module_rollback.test.js(5) + 8 new intest_plugin_src_route.py: identical bytes under the prefix; the whole graph one and two levels deep; assets (the [P2]); every plugin route; non-ASCII filenames (the [P3]); an opaque token; and containment asserted as parity with the un-prefixed route rather than a guessed 404 —../screen.jslegitimately 200s on both, because the URL normalises before routing ever happens.All bite-tested: reverting the fix fails the rollback tests; disabling the rewrite fails the asset tests.
Two harnesses re-anchored on
script.src = _pluginScriptUrl(— the URL literal they keyed on now lives in the helper, further down the file, so their slice ran off the end of the injection block.node 1045 · pytest 2404 · ESLint 0 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit