Skip to content

Improve MainModuleFactory type emitted by create_tsd #24087

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

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

Conversation

s-h-a-d-o-w
Copy link

@s-h-a-d-o-w s-h-a-d-o-w commented Apr 10, 2025

This is in accordance with the type on DefinitelyTyped.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@sbc100 sbc100 requested a review from brendandahl April 10, 2025 16:27
@sbc100
Copy link
Collaborator

sbc100 commented Apr 10, 2025

Looks you likely need to run some tests with --rebaseline

@s-h-a-d-o-w
Copy link
Author

Whoops, should've thought about there being tests for this. Sorry about that.

Since I will need to set up the whole toolchain to run that, it's going to take a bit though.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 10, 2025

The way I recommend doing it is emsdk install tot && emsdk activate tot then export EM_CONFIG=/path/to/emsdk/.emscripten

@s-h-a-d-o-w
Copy link
Author

Thanks but I'm on windows, without LLVM or clang. 😅

@sbc100
Copy link
Collaborator

sbc100 commented Apr 10, 2025

Thanks but I'm on windows, without LLVM or clang. 😅

emsdk install tot takes care of installing all that for you at the correct version.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 10, 2025

I'm happy to do that rebase for you if install emsdk is too much? (But how are you running emscripten then?

@s-h-a-d-o-w
Copy link
Author

Nah, it was just a oversight - I forgot to set the env variable like you told me to. It works great!

@s-h-a-d-o-w
Copy link
Author

s-h-a-d-o-w commented Apr 10, 2025

So... running with --rebaseline logs nothing to the console and changes no files.
It also finishes running very quickly. Should it?

I'm not confident that everything works as intended because when I run random tests, many fail, e.g.:

  File "C:\Users\andy\temp\emsdk\python\3.9.2-nuget_64bit\lib\unittest\case.py", line 670, in fail
    raise self.failureException(msg)
AssertionError: either d8 or node >= 24 required to run wasm64 tests.  Use EMTEST_SKIP_WASM64 to skip

Which is interesting given that "Current" node is 23.

Anyway... I only stumbled across this randomly and usually don't use emscripten, so it's probably easiest for both of us if you run that. Thanks for offering that!

sbc100 added 2 commits April 10, 2025 11:29
@sbc100
Copy link
Collaborator

sbc100 commented Apr 10, 2025

Actually I was chatting with @brendandahl and we think maybe the DefinitelyTyped definition is wrong here. We really want something that is a object/dictionary that matches INCOMING_MODULE_JS_API.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 10, 2025

We can maybe land this for now then move away from it.

BTW, are using using the generated types in combination with DefinitelyTyped somehow? Should we try to update to delete what is stored in DefinitelyTyped?

@s-h-a-d-o-w
Copy link
Author

No but thanks for asking!

(Long story: I arrived here from trying to improve the tree sitter TypeScript types, which are currently not in the best shape. I stumbled across this PR and thought that maybe it didn't get any attention because of the missing PR description or maybe because it didn't address the problem at the root, so I tried to.
I saw that the factory type in DefinitelyType has existed for at least 5 years and was discussed 5 years ago, so assumed that it was correct.
But then I realized that vemoo was right and the types in tree sitter weren't actually generated using emscripten. And that my problem was much simpler anyway and the type problems in tree sitter don't affect me. So I decided to leave these problems be and refocus on my own work.)

@sbc100
Copy link
Collaborator

sbc100 commented Apr 14, 2025

@brendandahl should file a bug to have https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/emscripten/index.d.ts removed? It seems like emscripten-generated types via --emit-tsc are always going to better, and anything checked into DefinitelyTyped likely never going to be up-to-date or match the interface of any specific emscripten output.

@brendandahl
Copy link
Collaborator

@brendandahl should file a bug to have https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/emscripten/index.d.ts removed? It seems like emscripten-generated types via --emit-tsc are always going to better, and anything checked into DefinitelyTyped likely never going to be up-to-date or match the interface of any specific emscripten output.

Yeah, I can do that. It looks like that file as some better typing information for a few interfaces that we could pull over and put in the JS doc comments for the library (which will then generate TS definitions).

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.

None yet

3 participants