fix: scan_external_changes never reloads script-bearing modules#223
Open
benpalevsky wants to merge 1 commit into
Open
fix: scan_external_changes never reloads script-bearing modules#223benpalevsky wants to merge 1 commit into
benpalevsky wants to merge 1 commit into
Conversation
Environment::scan_external_changes() previously skipped any module with a
script_class_id (`if (module->script_class_id) continue;`), so editing a
.ts/.js script on disk never re-executed its module and live GodotJSScript
instances kept their stale prototype. Remove the skip and return the set of
reloaded module ids as a Vector<StringName> so the language layer knows which
scripts to rebind.
A changed module's dependents must also reload. After collecting the directly
mtime/hash-dirty modules, fixed-point iterate the module graph: any module
whose `children` array references a dirty module is cascaded dirty via
force_mark_as_reloading(), which bypasses the mtime/md5 gate (its own source
bytes did not change) and bumps time_modified to stay coherent with the next
mark_as_reloading() probe.
Before re-running a module's wrapped source, reset `module.exports` to a fresh
Object. Compilers targeting CommonJS for an ES `export default` emit
`Object.defineProperty(exports, "default", { configurable: false, ... })`,
which throws "Cannot redefine property: default" on the second run while the
old non-configurable "default" still sits on exports. Without the reset the
reload silently fails for any module using ES-module-style default exports.
In the language layer, scan_external_changes() now narrows the rebind to the
dirty set returned by the environment rather than reloading every script. The
rebind snapshots script_list_ into Refs under the lock and iterates the
snapshot, not the live list: GodotJSScriptLanguage::mutex_ is a recursive
Mutex, so re-entrancy is not a self-deadlock; the real hazard is iterator
invalidation / use-after-free, because force_reload_for_scan ->
set_script(Ref<Script>()) can drop the last ref to a GodotJSScript whose
~GodotJSScript removes itself from script_list_ under the same recursive lock,
mutating the list mid-iteration. Holding a Ref per element keeps each script
alive across the loop.
Tests use raw .js fixtures (target.js, transitive-base/dependent.js,
script-class-target.js with the Object.defineProperty default-export pattern)
so they reproduce the bug and validate the fix without any TypeScript
transpiler.
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.
Problem
When you edit a script file on disk — a
.tsor.jsthat defines a Godot class — and switch back to the editor, GodotJS should notice the change and reload it so your running scene picks up the new code. It didn't.Two things were in the way:
Environment::scan_external_changes()deliberately skipped any module that was a script (there was a// skip script modules which are managed by the godot editorline), so a script edit never reached the reload path.GodotJSScriptLanguage::reload_tool_script(), is an empty stub.So editing a script class on disk never reloaded it — even though the underlying reload machinery (re-running the module and re-parsing the class) mostly already worked (modulo the
exports-redefinition trap fixed below).What this changes
scan_external_changes()return the list of module ids it reloaded.module.exportsbefore re-running. Whenswcoresbuildtargets CommonJS for anexport default, they emitObject.defineProperty(exports, "default", { enumerable: true, get: ... })withconfigurabledefaulting tofalse; on the second run that throwsCannot redefine property: default. (tscemits a plainexports.default = Xfor default exports, but hits the same non-configurable trap via the__createBindinghelper when re-exporting —export { x } from './y'.) Resettingexportsto a fresh object before re-running the module avoids the trap for any compiler.GodotJSScriptinstances are rebound — but only the ones whose module is in the reloaded ("dirty") set, not every script. The editor-only recovery pass (re-attaching a script whose.jswas missing at load) still runs on every scan, as before. The rebind snapshots the script list under the lock and iterates the snapshot, because a reload can drop a script and modify that list mid-walk.Tests
New tests under
tests/project/tests/reload/:Cannot redefine propertycase),They use plain
.jsfixtures, so they reproduce the bugs without needing any TypeScript transpiler. To drive the scan headlessly the tests call a small new internal binding,jsb.internal.scan_external_changes()(added in the bridge module loader), which routes through the language singleton so the rebind path is exercised too.Known follow-ups (not in this PR)
reload_tool_script()entry point is still a stub. Now that the scan path works, wiring it up is a reasonable separate change.Verbose-level[reload]diagnostic logs remain and could be trimmed.How to verify
Rebuild the engine, then in the editor edit a script that defines an exported class, switch focus back to the editor, and confirm the running scene and the inspector pick up the change (including new
@exportproperties) without a manual reload.A changeset is included.