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

Emscripten: Add EmscriptenModule interface type, make Module an instance of it. #36094

Merged
merged 1 commit into from Jun 27, 2019

Conversation

wffurr
Copy link
Contributor

@wffurr wffurr commented Jun 10, 2019

Fixes #24791. I plan to use this when sending code to Emscripten for generating TypeScript typings from embind bindings.

Note that this changed some type names:

  • Module.WebAssemblyImportsEmscripten.WebAssemblyImports
  • Module.WebAssemblyExportsEmscripten.WebAssemblyExports

This was required because Module is now an instance of the EmscriptenModule interface, and can't host type aliases.

@cpitclaudel @periklis please review and comment if this meets your needs.

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <>
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

Comparison details 📊
master #36094 diff
Batch compilation
Memory usage 80224816.0 80227936.0 0.0%
Type count 9020 9020 0.0%
Assignability cache size 32516 32516 0.0%
Subtype cache size 10 10 0.0%
Identity cache size 3 3 0.0%
Language service
Samples taken 159 159 0.0%
Identifiers in tests 159 159 0.0%
getCompletionsAtPosition
    Mean duration (ms) 362.3 355.4 -1.9%
    Median duration (ms) 361.3 351.0 -2.9%
    Mean CV 14.1% 13.0% -7.7%
    Worst duration (ms) 446.6 428.1 -4.1%
    Worst identifier parseInt Module
getQuickInfoAtPosition
    Mean duration (ms) 353.1 347.7 -1.5%
    Median duration (ms) 349.1 346.1 -0.9%
    Mean CV 13.5% 14.2% +5.1%
    Worst duration (ms) 438.5 434.1 -1.0%
    Worst identifier environment package

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.


If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 10, 2019

@wffurr Thank you for submitting this PR!

🔔 @zakki @periklis - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

// By default Emscripten emits a single global Module. Users setting -s
// MODULARIZE=1 -s EXPORT_NAME=MyMod should declare their own types, e.g.
// declare var MyMod: EmscriptenModule;
declare var Module: EmscriptenModule;
Copy link

Choose a reason for hiding this comment

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

Maybe we could omit this one, so that every person defines the correct module name. If one uses the default settings, Emscripten will indeed emit Module, but otherwise this will "pollute" the global types. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default Emscripten configuration uses the global Module name. It's unfortunate that it's so generic but I don't know of any specific conflicts with it. @periklis in the linked issue (#24791) claims that the MODULARIZE option with other names is not much used.

Of the two use cases I have encountered at work where we have just imported this typings definition, one uses Module and the other sets a name with MODULARIZE.

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'd prefer to keep the annotation in order to make using the typings definition simple for Emscripten users with the default set up.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Review in Pull Request Status Board Jun 15, 2019
@typescript-bot typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Jun 15, 2019
@typescript-bot
Copy link
Contributor

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

@uniqueiniquity
Copy link
Contributor

Congratulations on your first DefinitelyTyped contribution!
Thank you for being a part of the community!

@uniqueiniquity uniqueiniquity merged commit f2984e9 into DefinitelyTyped:master Jun 27, 2019
Pull Request Status Board automation moved this from Review to Done Jun 27, 2019
@typescript-bot
Copy link
Contributor

I just published @types/emscripten@0.0.33 to npm.

@wffurr
Copy link
Contributor Author

wffurr commented Jun 28, 2019 via email

iRON5 pushed a commit to iRON5/DefinitelyTyped that referenced this pull request Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Unmerged The author did not merge the PR when it was ready.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for MODULARIZE in emcripten definitions
4 participants