Conversation
There was a problem hiding this comment.
Pull request overview
Fixes markbind serve -d hot-reload regression by updating how the MarkBind Vue bundle is loaded in an ESM environment.
Changes:
- Replaces the previous in-memory
requireFromStringapproach with an ESM-compatible flow that writes the bundle to a temporary.cjsfile and imports it. - Updates the bundle hot-reload path to
awaitthe new async loader. - Adds a
.gitignoreentry forvue-module*.cjstemp artifacts.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/src/Page/PageVueServerRenderer.ts | Loads the regenerated Vue bundle via temp .cjs file + dynamic import to support ESM. |
| .gitignore | Ignores possible leftover temp .cjs files created by the new hot-reload loader. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2868 +/- ##
==========================================
+ Coverage 70.88% 70.96% +0.07%
==========================================
Files 131 131
Lines 7083 7085 +2
Branches 1668 1661 -7
==========================================
+ Hits 5021 5028 +7
+ Misses 1962 1957 -5
Partials 100 100 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
LGTM thanks for the quick fix. Tested on my machine and the fix works well.
Understanding:
-
Previous implementation relied on Node's CJS runtime internals,
module.constructorto create a new module instance,module.pathsto inherit the resolution paths, and_compile()to execute the source. None of these exist in ESM. -
New implementation: As bundle being loaded is still CJS format (core-web bundle), which expects
require,module, andexportsto exist. Provide these by usingcreateRequire(import.meta.url)forrequirefunction anchored to the current ESM file's location, so dependencies resolve correctly, a plainmodobject to stand in for module/exports and wrapping source in a new Function(...) call to execute it in the global scope, injecting the fake CJS environment as parameters
The mod.exports.default ?? mod.exports at the end also handles both CJS and ESM-compiled-to-CJS export shape.
Can add testcases, seem meaningful as it would catch such regressions and would probably be the first few testcases to cover -d command, but up to you since this is a hotfix
8d4c9b7 to
d0130c0
Compare
Markbind hot reload breaks due to the old import syntax used in updating the Markbind Vue Bundle. Change to ESM-compatible method of writing src to a temporary .cjs file that is imported using the file path. ESM only supports similar require functionality when using its experimental vm.Module object that can only be accessed when ran with a flag.
Implementation currently writes to a temp file, which is placed in the directory of the source code so that it can crawl the require tree. This may not work in production environments, especially if markbind-cli was installed globally as a root user and used as a normal user, as it wouldn't be able to write into the directory that markbind lives in. (additionally, we can't write anywhere else as it then wouldn't be able to crawl the dep tree) Use createRequire with an eval workaround to import it instead. This prevents the need of writing to a temp directory, similar to before. However, ESM still does not expose the `module` object, so the same method as before wouldn't work. Therefore, use Function() to import it, passing in the constructed require object.
requireFromString is untested and private Due to changes in build output, this method broke silently, highlighting importance of testing this section. Add unit tests checking import functionality.
| function requireFromString(src: string) { | ||
| // Use createRequire since bundle is CJS. This allows require() calls within the bundle | ||
| // to be resolved relative to this file. | ||
| const require = createRequire(import.meta.url); |
There was a problem hiding this comment.
Minor comment, would it be better to move this to the module scope? i.e. instantiate it outside of this function. A brief conversation with claude says:
The practical gains are modest since createRequire is cheap, but there are a few concrete reasons to prefer this:
Correctness — the binding URL (import.meta.url) is the same value every single call, so there's no reason to re-evaluate it. Hoisting makes that intent explicit.
Allocation — each call currently allocates a new require function object. If requireFromString is called frequently (e.g. per-page during SSR), this creates unnecessary GC pressure.
Clarity — a module-level require signals to readers that this is a stable, shared resolver, not something that varies per invocation. The current placement inside the function implies it might differ per call, which is misleading.
There was a problem hiding this comment.
Fair point. I'll do that real quick 👍
| // load dependencies. | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-implied-eval | ||
| new Function('require', 'module', 'exports', src)(require, mod, mod.exports); |
There was a problem hiding this comment.
this is really cool. u surprise me everyday with something new
What is the purpose of this pull request?
Overview of changes:
Fixes regression from #2863 due to migration to ESM environment
Markbind hot reload breaks due to the old import
syntax used in updating the Markbind Vue Bundle.
Change to ESM-compatible method by injecting require from
createRequireand module objects into the CJS src code so that it can evaluate and be used.Anything you'd like to highlight/discuss:
Testing instructions:
Run
markbind serve -dProposed commit message: (wrap lines at 72 characters)
Fix
markbind serve -dregressionChecklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):