Skip to content

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