Skip to content

[embind] Allow Closure to compress registerType methods #24610

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 1 commit into from
Jun 21, 2025

Conversation

RReverser
Copy link
Collaborator

The reason 'fromWireType', 'toWireType' and 'readValueFromPointer' were always protected from Closure is because we dynamically generate JS functions, and property mangling would make it impossible for the dynamically generated code to access properties declared statically.

However, we can easily work around that by binding corresponding methods outside of the generated lambdas, and passing just the methods instead of the whole type object.

The reason 'fromWireType', 'toWireType' and 'readValueFromPointer' were always protected from Closure is because we dynamically generate JS functions, and property mangling would make it impossible for the dynamically generated code to access properties declared statically.

However, we can easily work around that by binding corresponding methods outside of the generated lambdas, and passing just the methods instead of the whole type object.
@RReverser RReverser requested review from brendandahl and sbc100 June 19, 2025 15:55
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.

Great!

Are you sure these property names are not required as part of the public/external API?

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 if the tests pass

@RReverser
Copy link
Collaborator Author

Are you sure these property names are not required as part of the public/external API?

Fairly sure, as they are only needed for type registration, but I'll wait for @brendandahl to review as well.

@RReverser
Copy link
Collaborator Author

Actually, yeah, I rechecked, this JavaScript API is not part of our public API anywhere.

@RReverser RReverser merged commit 357a773 into emscripten-core:main Jun 21, 2025
30 checks passed
@RReverser RReverser deleted the embind-closure-friendly branch June 21, 2025 11:29
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.

2 participants