Skip to content

[file_packager.py] Add --export-es6 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.

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

Merged
merged 7 commits into from
Aug 13, 2025

Conversation

lkwinta
Copy link
Contributor

@lkwinta lkwinta commented Jul 18, 2025

I have added modularize option flag to file_packager.py script, like in main js generator
I think it will be helpful since in modern world of react it is inconvinent to add generated files as <script/> tags.
This pull request introduces a new --export-es6 option to the file_packager.py tool, enabling the generation of modularized JavaScript output. It enables to import generated JS loading stub into ES6 environment. The main module which will take effect of script execution is now passed as script argument.

Use scenario:

emcc -s MODULARIZE -s EXPORT_NAME=MainModule ...
file_packager.py --export-es6 ...
import MainModule from 'main_module_wasm'

// two datafiles that will be loaded at runtime with module
import { default as loadDataFile_specialName } from 'data_file_preload' 
import loadDataFile from `data_file_preload2`

var preMod = {
   // i. e. locateFile
}

var mod = MainModule(preMod)
mod.then(async (module) =>  {
    await loadDataFile(module);
    await loadDataFile_specialName(module;

    // module has now loaded data file in VFS
});

Fixes: #24504

@lkwinta lkwinta force-pushed the file_packager_modularize branch from c4a662f to deac6ec Compare July 22, 2025 20:00
@grzanka
Copy link

grzanka commented Jul 22, 2025

@sbc100 could you please take a look ?

@sbc100 sbc100 changed the title [file_packager.py] Add modularize to file_packager.py [file_packager.py] Add --modularize to file_packager.py Jul 22, 2025
@sbc100
Copy link
Collaborator

sbc100 commented Jul 22, 2025

Can you explain a little more in the PR description exactly what --modularize is doing here? It looks like its generating an ES6 module from the data files, is that right? How is the generated module supposed to interact with the program into which the files are to be loaded?

@lkwinta lkwinta requested a review from sbc100 July 22, 2025 21:29
@lkwinta
Copy link
Contributor Author

lkwinta commented Jul 22, 2025

Can you explain a little more in the PR description exactly what --modularize is doing here? It looks like its generating an ES6 module from the data files, is that right? How is the generated module supposed to interact with the program into which the files are to be loaded?

I have added some use case description.

As described in the related issue we are just trying to make the dynamic loading of large necessary dependencies inside web workers run from react application. Together with XHR lazy loading this seems to be only valid solution to our problem that might solve our problem.

@lkwinta lkwinta requested a review from sbc100 July 23, 2025 11:58
@lkwinta lkwinta force-pushed the file_packager_modularize branch 2 times, most recently from 1b74c3e to 05f9693 Compare July 23, 2025 21:41
@lkwinta
Copy link
Contributor Author

lkwinta commented Aug 2, 2025

@sbc100 what do you think about it now?

@lkwinta lkwinta force-pushed the file_packager_modularize branch from 47dcb43 to e2b3fc9 Compare August 2, 2025 09:56
@lkwinta lkwinta force-pushed the file_packager_modularize branch from a83bbfe to 58e06be Compare August 4, 2025 19:34
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Looks like we still need a test for this.

And might be worth adding to the changelog.

Otherwise lgtm.

@lkwinta lkwinta force-pushed the file_packager_modularize branch from 38f054e to 0a72d80 Compare August 4, 2025 21:08
@lkwinta
Copy link
Contributor Author

lkwinta commented Aug 4, 2025

Looks like we still need a test for this.

And might be worth adding to the changelog.

Otherwise lgtm.

I have added two simple tests - from what I have seen file_packager.py tests aren't really extensive.

@lkwinta lkwinta force-pushed the file_packager_modularize branch from 1e7790c to 149e8b3 Compare August 4, 2025 22:28
@lkwinta lkwinta force-pushed the file_packager_modularize branch 3 times, most recently from b011fb3 to fb98269 Compare August 5, 2025 11:58
@lkwinta lkwinta force-pushed the file_packager_modularize branch 2 times, most recently from e59ffa9 to 6f10cbb Compare August 8, 2025 13:56
@lkwinta lkwinta requested a review from sbc100 August 8, 2025 14:40
@lkwinta lkwinta force-pushed the file_packager_modularize branch 3 times, most recently from ad3b54c to be712ca Compare August 12, 2025 19:24
.catch((cause) => Promise.reject(cause))
.then(() => {
module._test_fun();
}
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 the .catch here. Doing return loadDataFile(.. should be enough to reject the promise if loadDataFile fails/rejects.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I think just using top-level-await might be more clear here:

var module = await loadModule();
await loadDataFile(module):
module._test_fun();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will apply that later, I have changed some logic to be more universal and to allow catching thrown error, take a look please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder, if I shouldn't add some throw error method to clear run dependencies - or maybe generate separate stub that allows to do that if we want ourselfs in external catch block

Copy link
Contributor Author

@lkwinta lkwinta Aug 12, 2025

Choose a reason for hiding this comment

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

I wonder, if I shouldn't add some throw error method to clear run dependencies - or maybe generate separate stub that allows to do that if we want ourselfs in external catch block

It could be other PR tho, I have just observed anoying behaviour that when it fails, main module constantly spams run dependencies message, being able to clean those to graceful escape error would be nice

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 that removing the run dependency is the way to fix that.. since that would result in the module running. If a dependency fails we want to block the running forever I think.. not just ignore it. But I think the existing code might not do that :(

@lkwinta lkwinta requested a review from sbc100 August 12, 2025 20:11
.catch((cause) => Promise.reject(cause))
.then(() => {
module._test_fun();
}
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 that removing the run dependency is the way to fix that.. since that would result in the module running. If a dependency fails we want to block the running forever I think.. not just ignore it. But I think the existing code might not do that :(

@lkwinta lkwinta force-pushed the file_packager_modularize branch from c0368f8 to 529e4a2 Compare August 12, 2025 20:55
@lkwinta lkwinta requested a review from sbc100 August 12, 2025 21:36
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % final comments

} else {
fetched = data;
}
})%s;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like there is no current error handling here, for this fetch. It looks like the code is trying to do a pre-fetch and maybe we can just ignore failures (like we currently do) for the pre-fetch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case this block could be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, without this catch here in es6 mode, i can't handle error on my own, the whole js crashes because of 'unhandled error on promise'

Fine:

async function F1() {
    throw new Error();
}

F1().catch((err)=>console.log(err))

Unhandled error in promise:

async function F1() {
    async function F2(){
    throw new Error();
    }
    F2();
}

F1().catch((err)=>console.log(err))

Copy link
Collaborator

Choose a reason for hiding this comment

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

But isn't that just the existing behaviour? There is not catch mechanism for this particular fetch and so errors here will always go the to the global error handlers. I think that is the current/expected behaviour. We could change that perhaps?

Given that there is not really any way to recover from such errors why not just let the global error handler handle these errors? Or the global unhandledRejection handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant that when the throw is two or more level deep I cant catch the error even with top level try catch block which is an issue - most likely es6 mode will be used together with some frontend and we don't want to crash whole JS but rather give some information and maybe possibility to retry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And inside webworker we can't access window context to get access to unhandledrejection handler

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can handle unhandled rejections using the worker global scope:

    // worker.js
    self.addEventListener('unhandledrejection', (event) => {
        console.error('Unhandled Promise Rejection in Worker:', event.reason);
    });

But I understand that it would be nice to correctly reject the promise here, so this part lgtm.

We can refactor later perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@lkwinta lkwinta force-pushed the file_packager_modularize branch from 2aa190d to e0bc809 Compare August 13, 2025 09:09
@lkwinta lkwinta requested a review from sbc100 August 13, 2025 13:29
@lkwinta
Copy link
Contributor Author

lkwinta commented Aug 13, 2025

So @sbc100 do we merge that?

@sbc100
Copy link
Collaborator

sbc100 commented Aug 13, 2025

Yup, LGTM.

I just landed #24917 so maybe worth rebasing/merging one more time.

fix python format

apply more ruff fixes

fix pipeline

revert file to the most actual version and apply changes

add modularize to options class

add utilization of EXPORT_NAME with modularize option

simplify declaration of the modularize function

arg documentation

address @sbc100 review

Fix codegen new line formatting

Add file_packager tests and changelog mention

fix some stylistic issues

gichange file_packager modularize test

simplyfi tests

fix test

rename to export_es6

async function promise behaviour

cleanup test

address @sbc100 review

simplify the test

rename promise handlers

introduce createRequire in es6 mode

alternative way to import require
@lkwinta lkwinta force-pushed the file_packager_modularize branch from 9241eca to 2874708 Compare August 13, 2025 20:01
@lkwinta
Copy link
Contributor Author

lkwinta commented Aug 13, 2025

Yup, LGTM.

I just landed #24917 so maybe worth rebasing/merging one more time.

done

@sbc100 sbc100 enabled auto-merge (squash) August 13, 2025 21:45
@sbc100 sbc100 merged commit 59c47d2 into emscripten-core:main Aug 13, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using standalone file_packager
3 participants