Skip to content
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

Add deleteLater() and setDelayFunction(...) to TypeScript definitions and make them official API #22701

Open
arnog-ms opened this issue Oct 9, 2024 · 14 comments

Comments

@arnog-ms
Copy link

arnog-ms commented Oct 9, 2024

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.68-git
clang version 20.0.0git
Target: wasm32-unknown-emscripten
Thread model: posix
InstalledDir: /opt/homebrew/Cellar/emscripten/3.1.68/libexec/llvm/bin

We are using the deleteLater() function a lot in our TypeScript codebase to not forget to delete a short lived reference. This works quite well. The only drawback is that we have to patch the generated TypeScript definitions to include the deleteLater() function for each interface that also has the delete() function. And we patch the MainModule export to include the setDelayFunction(...) definition, so that we can set the delay function at MainModule instantiation.

It would be great, if these two could be added to the generated TypeScript definitions and also be made an official API.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 9, 2024

I believe if you want to have setDelayFunction exported you should add it to EXPORTED_RUNTIME_METHODS. This should result it being part of the module definition.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 9, 2024

It looks like we were exported some functions under the hood so setDelayFunction was being made available, even if it wasn't needed. I'm fixing that in #22705.

Regarding deleteLater, that looks like maybe a different issue. @brendandahl should be declaring these methods in the generated TS?

@arnog-ms
Copy link
Author

I tried to add -sEXPORTED_RUNTIME_METHODS=setDelayFunction but received the following warning

warning: invalid item in EXPORTED_RUNTIME_METHODS: setDelayFunction
em++: warning: warnings in JS library compilation [-Wjs-compiler]

Is this due to the bug mentioned above? If so, should this work with 3.6.19 once it's released?

@sbc100
Copy link
Collaborator

sbc100 commented Oct 10, 2024

Thats odd. Are you passing --bind? Is seems to work fine for me:

((3.1.68)) $ ./emcc test/hello_world.c -sEXPORTED_RUNTIME_METHODS=setDelayFunction --bind

@arnog-ms
Copy link
Author

arnog-ms commented Oct 10, 2024

This is an excerpt from our cmake file. -lembind is equivalent to --bind, isn't it?

if(${CMAKE_SYSTEM_NAME} MATCHES "Emscripten")
    set(CMAKE_EXECUTABLE_SUFFIX ".js")

    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --no-entry --emit-tsd wasmtypes.d.ts")

    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -sWASM=1 -sEXPORT_ES6=1 -sMODULARIZE=1 -sEXPORTED_RUNTIME_METHODS=setDelayFunction -lembind -sEXPORT_NAME=ManagerModule")
endif()

@brendandahl
Copy link
Collaborator

I think that is hitting the bug about runtime exports combined with emit-tsd. That is fixed #22705

@sbc100
Copy link
Collaborator

sbc100 commented Oct 10, 2024

Ah yes, try emsdk install tot to get ta build with that change in.

@arnog-ms
Copy link
Author

OK, I will give that a try in the next few days.

If I understand correctly, this would only solve the setDelayFunction export, right? So the deleteLater is not covered by this?

@brendandahl
Copy link
Collaborator

Correct, I'll add a separate fix for that.

@arnog-ms
Copy link
Author

Thank you very much!

@sbc100
Copy link
Collaborator

sbc100 commented Oct 10, 2024

Yes setDelayFunction is just a normal library function so should be part of your generated TS once its exported normally.

deleteLater looks like its a method that exists on all embind objects. Should we be declaring this (and other builtin methods)? Is it possible for a user method to collide with one of these builtin method names?

@arnog-ms
Copy link
Author

I think, at the moment, only delete is exported automatically. For me it would be great to also export deleteLater automatically.

Together with setDelayFuntion this makes it much easier to not forget to delete a reference.

It might also be worthwile to set the delay function to setTimeout(fn, 0) by default.

@brendandahl
Copy link
Collaborator

Yes setDelayFunction is just a normal library function so should be part of your generated TS once its exported normally.

deleteLater looks like its a method that exists on all embind objects. Should we be declaring this (and other builtin methods)? Is it possible for a user method to collide with one of these builtin method names?

I think we just need to update the defaults. These definitions were mostly from the previous TS project, so I'm not sure why they're missing. The names could collide, but that should really be an error.

@arnog-ms
Copy link
Author

arnog-ms commented Oct 14, 2024

I tested with 3.1.69 and I can see the export of setDelayFunction in the .d.ts file. Thanks for that. :-)

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

No branches or pull requests

3 participants