fix: Improved node_modules handling during game exports.

- Internally, scripts now retain their module dependency hierarchy,
  even when the module being loaded was previously cached. Prior
  to this fix, scripts incorrectly only stored dependencies that
  were not previously encountered as a dependency of another
  script.
- .cjs extension is now more widely support. Previously support
  was limited to being imported from a GodotJS script, but .cjs
  files themselves were not considered scripts. This caused .cjs
  files contained within node_modules to be incorrectly omitted
  during export since they weren't recognized as scripts.
- No longer blingly bundling transitive node_modules. Really, we
  don't support transitive node_modules. node_modules should be
  hoisted.

Author: Benjamin-Dobell

.changeset/moody-cars-sell.md

@@ -0,0 +1,5 @@
+---
+"@godot-js/editor": patch
+---
+
+fix: Improved node_modules handling during game exports.

bridge/jsb_environment.cpp

@@ -1058,10 +1058,10 @@ namespace jsb
     JavaScriptModule* Environment::_load_module(const String& p_parent_id, const String& p_module_id)
     {
         JSB_BENCHMARK_SCOPE(JSRealm, _load_module);
-        JavaScriptModule* existing_module = module_cache_.find(p_module_id);
-        if (existing_module && !existing_module->is_reloading())
+        JavaScriptModule* resolved_module = module_cache_.find(p_module_id);
+        if (resolved_module && !resolved_module->is_reloading())
         {
-            return existing_module;
+            return resolved_module;
         }
 
         v8::Isolate* isolate = this->isolate_;
@@ -1071,7 +1071,7 @@ namespace jsb
         // find loader with the module id
         if (IModuleLoader* loader = this->find_module_loader(p_module_id))
         {
-            jsb_checkf(!existing_module, "module loader does not support reloading");
+            jsb_checkf(!resolved_module, "module loader does not support reloading");
             JavaScriptModule& module = module_cache_.insert(isolate, context, p_module_id, false, false);
 
             //NOTE the loader should throw error if failed
@@ -1107,35 +1107,37 @@ namespace jsb
             const StringName module_id = source_info.source_filepath;
 
             // check again with the resolved module_id
-            existing_module = module_cache_.find(module_id);
-            if (existing_module && !existing_module->is_reloading())
-            {
-                return existing_module;
-            }
+            resolved_module = module_cache_.find(module_id);
+
+            v8::Local<v8::Object> module_obj;
 
             // supported module properties: id, filename, cache, loaded, exports, children
-            if (existing_module)
+            if (resolved_module)
             {
-                jsb_check(existing_module->id == module_id);
-                jsb_check(existing_module->source_info.source_filepath == source_info.source_filepath);
-
-                JSB_LOG(VeryVerbose, "reload module %s", module_id);
-                existing_module->mark_as_reloaded();
-                if (!resolver->load(this, source_info.source_filepath, *existing_module))
+                if (resolved_module->is_reloading())
                 {
-                    return nullptr;
+                    jsb_check(resolved_module->id == module_id);
+                    jsb_check(resolved_module->source_info.source_filepath == source_info.source_filepath);
+
+                    JSB_LOG(VeryVerbose, "reload module %s", module_id);
+                    resolved_module->mark_as_reloaded();
+                    if (!resolver->load(this, source_info.source_filepath, *resolved_module))
+                    {
+                        return nullptr;
+                    }
+                    ScriptClassInfo::_parse_script_class(context, *resolved_module);
                 }
-                ScriptClassInfo::_parse_script_class(context, *existing_module);
-                return existing_module;
+
+                module_obj = resolved_module->module.Get(isolate);
             }
             else
             {
                 JSB_LOG(Verbose, "instantiating module %s", module_id);
                 JavaScriptModule& module = module_cache_.insert(isolate, context, module_id, true, false);
                 v8::Local<v8::Object> exports_obj = v8::Object::New(isolate);
-                v8::Local<v8::Object> module_obj = module.module.Get(isolate);
 
                 // init the new module obj
+                module_obj = module.module.Get(isolate);
                 module_obj->Set(context, jsb_name(this, children), v8::Array::New(isolate)).Check();
                 module_obj->Set(context, jsb_name(this, exports), exports_obj).Check();
                 module.source_info = source_info;
@@ -1148,29 +1150,6 @@ namespace jsb
                     return nullptr;
                 }
 
-                // build the module tree
-                if (!p_parent_id.is_empty())
-                {
-                    if (const JavaScriptModule* parent_ptr = module_cache_.find(p_parent_id))
-                    {
-                        const v8::Local<v8::Object> parent_module = parent_ptr->module.Get(isolate);
-                        if (v8::Local<v8::Value> temp; parent_module->Get(context, jsb_name(this, children)).ToLocal(&temp) && temp->IsArray())
-                        {
-                            const v8::Local<v8::Array> children = temp.As<v8::Array>();
-                            const uint32_t children_num = children->Length();
-                            children->Set(context, children_num, module_obj).Check();
-                        }
-                        else
-                        {
-                            JSB_LOG(Error, "can not access children on '%s'", p_parent_id);
-                        }
-                    }
-                    else
-                    {
-                        JSB_LOG(Warning, "parent module not found with the name '%s'", p_parent_id);
-                    }
-                }
-
                 module.on_load(isolate, context);
                 {
                     const impl::TryCatch try_catch_run(isolate);
@@ -1180,8 +1159,34 @@ namespace jsb
                         JSB_LOG(Error, "something wrong when parsing '%s'\n%s", module_id, BridgeHelper::get_exception(try_catch_run));
                     }
                 }
-                return &module;
+
+                resolved_module = &module;
+            }
+
+            // build the module tree
+            if (!p_parent_id.is_empty())
+            {
+                if (const JavaScriptModule* parent_ptr = module_cache_.find(p_parent_id))
+                {
+                    const v8::Local<v8::Object> parent_module = parent_ptr->module.Get(isolate);
+                    if (v8::Local<v8::Value> temp; parent_module->Get(context, jsb_name(this, children)).ToLocal(&temp) && temp->IsArray())
+                    {
+                        const v8::Local<v8::Array> children = temp.As<v8::Array>();
+                        const uint32_t children_num = children->Length();
+                        children->Set(context, children_num, module_obj).Check();
+                    }
+                    else
+                    {
+                        JSB_LOG(Error, "can not access children on '%s'", p_parent_id);
+                    }
+                }
+                else
+                {
+                    JSB_LOG(Warning, "parent module not found with the name '%s'", p_parent_id);
+                }
             }
+
+            return resolved_module;
         }
 
         impl::Helper::throw_error(isolate, jsb_format("unknown module: %s", normalized_id));

internal/jsb_path_util.cpp

@@ -95,12 +95,14 @@ namespace jsb::internal
 
     String PathUtil::convert_javascript_path(const String& p_source_path)
     {
-        if (p_source_path.ends_with("." JSB_JAVASCRIPT_EXT))
+        bool is_js_extension = p_source_path.ends_with("." JSB_JAVASCRIPT_EXT);
+        if (is_js_extension || p_source_path.ends_with("." JSB_COMMONJS_EXT))
         {
+            int extension_length = is_js_extension ? 3 : 4;
             const String root_path = Settings::get_jsb_out_res_path();
             jsb_checkf(p_source_path.begins_with(root_path), "can not proceed javascript sources not under the project data directory");
             const String replaced = String("res://").path_join(
-                p_source_path.substr(root_path.length() + 1, p_source_path.length() - root_path.length() - 3)
+                p_source_path.substr(root_path.length() + 1, p_source_path.length() - root_path.length() - extension_length)
                 + JSB_TYPESCRIPT_EXT);
             return replaced;
         }

internal/jsb_settings.cpp

@@ -79,7 +79,7 @@ namespace jsb::internal
         if (!inited)
         {
             inited = true;
-            static constexpr char filter[] = "*." JSB_JAVASCRIPT_EXT
+            static constexpr char filter[] = "*." JSB_JAVASCRIPT_EXT ",*." JSB_COMMONJS_EXT
 #if JSB_USE_TYPESCRIPT
                                             ",*." JSB_TYPESCRIPT_EXT
 #endif

weaver-editor/jsb_editor_plugin.cpp

@@ -436,7 +436,7 @@ void GodotJSEditorPlugin::collect_invalid_files(const String& p_path, Vector<Str
         }
         else
         {
-            if (!it_path.ends_with("." JSB_JAVASCRIPT_EXT) || !FileAccess::exists(jsb::internal::PathUtil::convert_javascript_path(it_path)))
+            if (!(it_path.ends_with("." JSB_JAVASCRIPT_EXT) || it_path.ends_with("." JSB_COMMONJS_EXT)) || !FileAccess::exists(jsb::internal::PathUtil::convert_javascript_path(it_path)))
             {
                 // invalid if it's not a source map file, or no corresponding .js file exist
                 if (!it_path.ends_with("." JSB_JAVASCRIPT_EXT ".map") || !FileAccess::exists(it_path.substr(0, it_path.length() - 4)))

weaver-editor/jsb_export_plugin.cpp

@@ -45,7 +45,7 @@ void GodotJSExportPlugin::export_raw_files(const PackedStringArray &p_paths, boo
     }
 }
 
-void GodotJSExportPlugin::get_script_resources(const String &p_dir, Vector<String> &r_list)
+void GodotJSExportPlugin::get_script_resources(const String &p_dir, Vector<String> &r_list, bool p_is_node_module)
 {
     Ref<DirAccess> dir = DirAccess::open(p_dir);
 
@@ -60,7 +60,7 @@ void GodotJSExportPlugin::get_script_resources(const String &p_dir, Vector<Strin
 
     while (!filename.is_empty())
     {
-        if (filename == "." || filename == "..")
+        if (filename == "." || filename == ".." || (p_is_node_module && filename == "node_modules"))
         {
             filename = dir->get_next();
             continue;
@@ -70,7 +70,7 @@ void GodotJSExportPlugin::get_script_resources(const String &p_dir, Vector<Strin
 
         if (dir->current_is_dir())
         {
-            get_script_resources(path, r_list);
+            get_script_resources(path, r_list, p_is_node_module);
         }
         else if (ResourceLoader::get_resource_type(path) == jsb_typename(GodotJSScript) && !get_ignored_paths().has(path))
         {
@@ -175,7 +175,7 @@ bool GodotJSExportPlugin::export_compiled_script(const String& p_path)
 
         String package_path = p_path.substr(0, package_path_slash_index);
         Vector<String> script_paths;
-        get_script_resources(package_path, script_paths);
+        get_script_resources(package_path, script_paths, true);
 
         const String package_json_path = jsb::internal::PathUtil::combine(package_path, "package.json");
 

weaver-editor/jsb_export_plugin.h

@@ -34,7 +34,7 @@ class GodotJSExportPlugin: public EditorExportPlugin
     bool export_module_files(const jsb::JavaScriptModule& p_module);
     bool export_raw_file(const String& p_path);
     void export_raw_files(const PackedStringArray& p_paths, bool p_permit_typescript);
-    void get_script_resources(const String &p_dir, Vector<String> &r_list);
+    void get_script_resources(const String &p_dir, Vector<String> &r_list, bool p_is_node_module = false);
 
     HashSet<String> exported_paths_;
     std::shared_ptr<jsb::Environment> env_;

weaver/jsb_resource_loader.cpp

@@ -13,6 +13,7 @@ namespace
         || p_path.ends_with(".worker." JSB_TYPESCRIPT_EXT)
 #   endif
         || p_path.ends_with(".worker." JSB_JAVASCRIPT_EXT)
+        || p_path.ends_with(".worker." JSB_COMMONJS_EXT)
 #endif
         ;
     }
@@ -25,6 +26,7 @@ namespace
         || p_path.ends_with(".test." JSB_TYPESCRIPT_EXT)
 #   endif
         || p_path.ends_with(".test." JSB_JAVASCRIPT_EXT)
+        || p_path.ends_with(".test." JSB_COMMONJS_EXT)
 #endif
         ;
     }
@@ -67,7 +69,7 @@ Ref<Resource> ResourceFormatLoaderGodotJSScript::load(const String& p_path, cons
         return {};
     }
 #endif
-    jsb_check(p_path.ends_with(JSB_TYPESCRIPT_EXT) || p_path.ends_with(JSB_JAVASCRIPT_EXT));
+    jsb_check(p_path.ends_with(JSB_TYPESCRIPT_EXT) || p_path.ends_with(JSB_JAVASCRIPT_EXT) || p_path.ends_with(JSB_COMMONJS_EXT));
 
     // in case `node_modules` is not ignored (which is not expected though), we do not want any GodotJSScript to be generated from it.
     if (p_path.begins_with("res://node_modules"))
@@ -129,6 +131,7 @@ void ResourceFormatLoaderGodotJSScript::get_recognized_extensions(List<String>*
     p_extensions->push_back(JSB_TYPESCRIPT_EXT);
 #endif
     p_extensions->push_back(JSB_JAVASCRIPT_EXT);
+    p_extensions->push_back(JSB_COMMONJS_EXT);
 }
 
 bool ResourceFormatLoaderGodotJSScript::handles_type(const String& p_type) const
@@ -141,9 +144,9 @@ String ResourceFormatLoaderGodotJSScript::get_resource_type(const String& p_path
     const String el = p_path.get_extension().to_lower();
 
 #if JSB_USE_TYPESCRIPT
-    if (el == JSB_TYPESCRIPT_EXT || el == JSB_JAVASCRIPT_EXT)
+    if (el == JSB_TYPESCRIPT_EXT || el == JSB_JAVASCRIPT_EXT || el == JSB_COMMONJS_EXT)
 #else
-    if (el == JSB_JAVASCRIPT_EXT)
+    if (el == JSB_JAVASCRIPT_EXT || el == JSB_COMMONJS_EXT)
 #endif // JSB_USE_TYPESCRIPT
     {
         return !is_worker_script(p_path) && !is_test_script(p_path)

weaver/jsb_resource_saver.cpp

@@ -38,6 +38,7 @@ void ResourceFormatSaverGodotJSScript::get_recognized_extensions(const Ref<Resou
         p_extensions->push_back(JSB_TYPESCRIPT_EXT);
 #endif
         p_extensions->push_back(JSB_JAVASCRIPT_EXT);
+        p_extensions->push_back(JSB_COMMONJS_EXT);
     }
 }
 

weaver/jsb_script_language.cpp

@@ -316,6 +316,7 @@ void GodotJSScriptLanguage::get_recognized_extensions(List<String>* p_extensions
     p_extensions->push_back(JSB_TYPESCRIPT_EXT);
 #endif
     p_extensions->push_back(JSB_JAVASCRIPT_EXT);
+    p_extensions->push_back(JSB_COMMONJS_EXT);
 }