Updates Emscripten types with new MODULARIZE API by lourd · Pull Request #44590 · DefinitelyTyped/DefinitelyTyped

lourd · 2020-05-09T00:20:40Z

Don't merge until the new release of Emscripten has been cut

This corresponds with the changes to Emscripten in emscripten-core/emscripten#10697,
creating a type for the new Promise-based API of the factory function
generated when using MODULARIZE.

Removes the obsolete then method from EmscriptenModule and separates
the type for the factory function generated when using the MODULARIZE
build option from the rest of the EmscriptenModule interface. This
will make it easier to extend the interface.

Also removes the global Module variable declaration as this is not
applicable for every application. This was not ideal as it left a very
generic variable name in the global scope for any application that used
Emscripten, regardless of whether there was actually a global variable.
This now leaves it up to the user to declare their module instance as
appropriate for them.

Fixes the reference in sql.js, using the EmscriptenModule interface
directly.

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).
Provide a URL to documentation or source code which provides context for the suggested changes: Implements Promise-based API for modularized builds emscripten-core/emscripten#10697
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
Include tests for your changes

This corresponds with the changes to Emscripten in emscripten-core/emscripten#10697, creating a type for the new Promise-based API of the factory function generated when using `MODULARIZE`. Removes the obsolete `then` method from EmscriptenModule and separates the type for the factory function generated when using the `MODULARIZE` build option from the rest of the `EmscriptenModule` interface. This will make it easier to extend the interface. Also removes the global `Module` variable declaration as this is not applicable for every application. This was not ideal as it left a very generic variable name in the global scope for any application that used Emscripten, regardless of whether there was actually a global variable. This now leaves it up to the user to declare their module instance as appropriate for them. Fixes the reference in sql.js, using the EmscriptenModule interface directly.

typescript-bot · 2020-05-09T00:20:51Z

@lourd Thank you for submitting this PR!

Code Reviews

Because this PR edits multiple packages, it can be merged once it's reviewed by a DT maintainer.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
❌ Only a DT maintainer can merge changes without tests

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 44590,
  "author": "lourd",
  "owners": [
    "zakki",
    "periklis",
    "kbumsik",
    "Hozuki",
    "ffflorian"
  ],
  "dangerLevel": "MultiplePackagesEdited",
  "headCommitAbbrOid": "cdce35b",
  "headCommitOid": "cdce35b4d471b29711ba7fee186c95d52c18cea4",
  "mergeIsRequested": false,
  "stalenessInDays": 6,
  "lastCommitDate": "2020-05-09T00:21:06.000Z",
  "lastCommentDate": "2020-05-15T17:21:38.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44590/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": false,
  "popularityLevel": "Well-liked by everyone",
  "anyPackageIsNew": false,
  "packages": [
    "emscripten",
    "sql.js"
  ],
  "files": [
    {
      "filePath": "types/emscripten/emscripten-tests.ts",
      "kind": "test",
      "package": "emscripten"
    },
    {
      "filePath": "types/emscripten/index.d.ts",
      "kind": "definition",
      "package": "emscripten"
    },
    {
      "filePath": "types/sql.js/module.d.ts",
      "kind": "definition",
      "package": "sql.js"
    }
  ],
  "hasDismissedReview": false,
  "travisResult": "pass",
  "lastReviewDate": "2020-05-09T01:23:28.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

typescript-bot · 2020-05-09T00:20:52Z

🔔 @zakki @periklis @kbumsik @hozuki @ffflorian - 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.

typescript-bot · 2020-05-09T00:20:54Z

🔔 @zakki @periklis @kbumsik @hozuki @ffflorian - 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.

typescript-bot · 2020-05-09T00:48:50Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

emscripten/v*

Comparison details for emscripten/* 📊

	master	#44590	diff
Batch compilation
Memory usage (MiB)	84.2	73.6	-12.6%
Type count	10378	10486	+1%
Assignability cache size	34516	34521	0%

Language service
Samples taken	268	297	+11%
Identifiers in tests	268	297	+11%
`getCompletionsAtPosition`
Mean duration (ms)	297.7	299.4	+0.6%
Mean CV	12.0%	11.7%
Worst duration (ms)	366.6	379.0	+3.4%
Worst identifier	strPtr	createModule
`getQuickInfoAtPosition`
Mean duration (ms)	285.5	289.0	+1.2%
Mean CV	10.7%	10.6%	-1.4%
Worst duration (ms)	354.3	361.8	+2.1%
Worst identifier	_free	unlink

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

sql.js/v*

Comparison details for sql.js/* 📊

	master	#44590	diff
Batch compilation
Memory usage (MiB)	103.3	108.5	+5.0%
Type count	15539	15538	0%
Assignability cache size	33895	33895	0%

Language service
Samples taken	104	104	0%
Identifiers in tests	104	104	0%
`getCompletionsAtPosition`
Mean duration (ms)	531.4	532.5	+0.2%
Mean CV	10.4%	10.2%
Worst duration (ms)	690.8	666.8	-3.5%
Worst identifier	each	selectRecordsStatement
`getQuickInfoAtPosition`
Mean duration (ms)	528.5	533.8	+1.0%
Mean CV	9.7%	10.3%	+5.6%
Worst duration (ms)	636.1	633.5	-0.4%
Worst identifier	b	db

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

kbumsik

Thanks for improving @types/emscripten!
I glad to see then() is finally removed. While reading emscripten-core/emscripten#10697 I am surprised by the amount of effort just to make it a real promise. That's much more than I'd imagine. Thanks so much!

I also like the idea of removing Module in favor of EmscriptenModuleFactory. LGTM.

hozuki

The sql.js part has the same type reference as before. As long as emscripten's definitions are correct, sql.js' def works too.

sandersn · 2020-05-13T15:26:13Z

@lourd emscripten-core/emscripten#10697 is merged. Is this PR ready to go? If not, which version is it waiting on, and where can I go to check whether that version has been released?

kbumsik · 2020-05-13T15:31:59Z

@sandersn The PR is merged but it is not released yet. Specifically it is waiting for 1.39.16 and the current release is 1.39.15. You can check it here: https://github.com/emscripten-core/emscripten/releases
Emscripten releases every 1 or 2 weeks so it will be soon I guess. I will leave a comment when it is released.

lourd · 2020-05-15T17:21:38Z

1.39.16 was just released this morning, this should be good to go!

typescript-bot · 2020-05-15T17:29:34Z

I just published @types/[email protected] to npm.

kbumsik · 2020-05-24T22:53:50Z

Just mentioning emscripten-core/emscripten#10271 to keep track of it.

* Updates Emscripten types with new `MODULARIZE` API This corresponds with the changes to Emscripten in emscripten-core/emscripten#10697, creating a type for the new Promise-based API of the factory function generated when using `MODULARIZE`. Removes the obsolete `then` method from EmscriptenModule and separates the type for the factory function generated when using the `MODULARIZE` build option from the rest of the `EmscriptenModule` interface. This will make it easier to extend the interface. Also removes the global `Module` variable declaration as this is not applicable for every application. This was not ideal as it left a very generic variable name in the global scope for any application that used Emscripten, regardless of whether there was actually a global variable. This now leaves it up to the user to declare their module instance as appropriate for them. Fixes the reference in sql.js, using the EmscriptenModule interface directly. * Runs prettier on the emscripten types

Louis DeScioli added 2 commits May 8, 2020 20:14

Runs prettier on the emscripten types

cdce35b

typescript-bot added the Where is Travis? label May 9, 2020

typescript-bot removed the Where is Travis? label May 9, 2020

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label May 9, 2020

kbumsik approved these changes May 9, 2020

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label May 9, 2020

hozuki approved these changes May 9, 2020

View reviewed changes

sandersn merged commit 32b5dfe into DefinitelyTyped:master May 15, 2020

s-h-a-d-o-w mentioned this pull request Apr 10, 2025

Improve MainModuleFactory type emitted by create_tsd emscripten-core/emscripten#24087

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates Emscripten types with new MODULARIZE API#44590

Updates Emscripten types with new MODULARIZE API#44590
sandersn merged 2 commits intoDefinitelyTyped:masterfrom
lourd:emscripten-factory-types

lourd commented May 9, 2020

Uh oh!

typescript-bot commented May 9, 2020 •

edited

Loading

Uh oh!

typescript-bot commented May 9, 2020

Uh oh!

typescript-bot commented May 9, 2020

Uh oh!

typescript-bot commented May 9, 2020

Uh oh!

kbumsik left a comment •

edited

Loading

Uh oh!

hozuki left a comment

Uh oh!

sandersn commented May 13, 2020

Uh oh!

kbumsik commented May 13, 2020 •

edited

Loading

Uh oh!

lourd commented May 15, 2020

Uh oh!

typescript-bot commented May 15, 2020

Uh oh!

kbumsik commented May 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

lourd commented May 9, 2020

Uh oh!

typescript-bot commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Reviews

Status

Uh oh!

typescript-bot commented May 9, 2020

Uh oh!

typescript-bot commented May 9, 2020

Uh oh!

typescript-bot commented May 9, 2020

emscripten/v*

sql.js/v*

Uh oh!

kbumsik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hozuki left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn commented May 13, 2020

Uh oh!

kbumsik commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lourd commented May 15, 2020

Uh oh!

typescript-bot commented May 15, 2020

Uh oh!

kbumsik commented May 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

typescript-bot commented May 9, 2020 •

edited

Loading

kbumsik left a comment •

edited

Loading

kbumsik commented May 13, 2020 •

edited

Loading