Skip to content

[file_packager.py] Add --modularize to file_packager.py #24737

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[file_packager.py] Add --modularize to file_packager.py #24737

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c6c74d0
Add modularize to file_packager.py
lkwinta Jul 18, 2025
d18dd68
fix python format
lkwinta Jul 18, 2025
3f3d8bf
apply more ruff fixes
lkwinta Jul 18, 2025
9914e14
fix pipeline
lkwinta Jul 18, 2025
738a4c2
revert file to the most actual version and apply changes
lkwinta Jul 18, 2025
a4eb650
add modularize to options class
lkwinta Jul 18, 2025
ee72cab
add utilization of EXPORT_NAME with modularize option
lkwinta Jul 23, 2025
05f9693
simplify declaration of the modularize function
lkwinta Jul 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions tools/file_packager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ def __init__(self):
self.use_preload_plugins = False
self.support_node = True
self.wasm64 = False
self.modularize = False


class DataFile:
Expand Down Expand Up @@ -391,6 +392,9 @@ def main(): # noqa: C901, PLR0912, PLR0915
elif arg == '--no-force':
options.force = False
leading = ''
elif arg == '--modularize':
options.modularize = True
leading = ''
elif arg == '--use-preload-cache':
options.use_preload_cache = True
leading = ''
Expand Down Expand Up @@ -616,13 +620,25 @@ def generate_js(data_target, data_files, metadata):
if options.from_emcc:
ret = ''
else:
ret = '''
if options.modularize:
ret = '''
export default function loadDataFile(moduleArg = {}) {
var Module = moduleArg;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this extra inner function here or can we just do:

export default loadDataFile(Module) {
  ...
}

Also, I think the function doesn't need to be async because it hooks into the Module object using addRunDependency / removeRunDependency. Although if the Module is already loaded and running that might not work, so we might need to change that too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function is async because I didn't touch inner code, i. e. web requests which return promises but I agree that it might need changing.

The inner function prevents from using new keywoard, just like in emcc ES 6 export code

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need the generated code here to look like the emscripten-generate program. For example, the file loading code just take a single Module argument, I think, it doesn't need the whole moduleArg dictionary thing.

Also, the emcc output no longer contains the new keyword check. See #23960.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, my bad, I was basing on emsdk release which doesn't have those changs.

What do you suggest to pass inside that function? Only Module's FS and used stuff like locate file?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the generated code expected something called Module which is the whole module object? But I could be wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also call it with your module args object and it looks like it will inject a Module.preRun in that case. i.e. it works if its passed either module args or that actual module.

Copy link
Author

@lkwinta lkwinta Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the generated code expected something called Module which is the whole module object? But I could be wrong.

Exactly, generated module want's to use modules function like locateFile and FS_CreatePath. Probably it messes with modules internal state while creating files in VFS.

You mean that I could just use it like this:

var preMod = {...}

var mod = MainModule(preMod);
 
createModule(preMod); //function from fille packager generated code

Isn't it the same as calling it with mod arg since MainModule(preMod) takes preMod by reference and extends it with fields? Od that's not the case I think we can't use it like this because we need to fill in Module's (mod) virtual FS paths.

Original use case was:

<script>
var preMod = { .... }
</script>
<script src="load_wasm"/> 
<script src="load_datafile"/>
<script>
// use Module to access wasm code
</script>

since it oprrates on the same global context IT passes whole Module dictionary.

In @kripken's demo linked in emscripten docs, DOM manipulation is used to insert <script> tags at runtime to load datafile, but we can't use it because of react and WebWorkers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to use EXPORT_NAME here.. can't you just always call the function loadDataFile? Since it exported by default I don't even think it really matter what it is called.

Why not just?

export default loadDataFile(Module) {
  ...
}

Also, as I think I said already I don't think need the inner function here do you? And I don't think anything need to be async because we never return a promise.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, now I see your point, I think that I have simplified it now.

I will take a look into those tests that are failing for some reason.

'''

else:
ret = '''
var Module = typeof %(EXPORT_NAME)s != 'undefined' ? %(EXPORT_NAME)s : {};\n''' % {"EXPORT_NAME": options.export_name}

ret += '''
Module['expectedDataFileDownloads'] ??= 0;
Module['expectedDataFileDownloads']++;
(() => {
Module['expectedDataFileDownloads']++;'''

if not options.modularize:
ret += '''
(() => {'''

ret += '''
// Do not attempt to redownload the virtual filesystem data when in a pthread or a Wasm Worker context.
var isPthread = typeof ENVIRONMENT_IS_PTHREAD != 'undefined' && ENVIRONMENT_IS_PTHREAD;
var isWasmWorker = typeof ENVIRONMENT_IS_WASM_WORKER != 'undefined' && ENVIRONMENT_IS_WASM_WORKER;
Expand Down Expand Up @@ -1148,7 +1164,11 @@ def generate_js(data_target, data_files, metadata):
}
loadPackage(%s);\n''' % json.dumps(metadata)

ret += '''
if options.modularize and not options.from_emcc:
ret += '''
};'''
else:
ret += '''
})();\n'''

return ret
Expand Down