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

feat(sourcemapconsumerSync): Synchronous sourcemap consumer interface #369

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

kwonoj
Copy link

@kwonoj kwonoj commented Oct 31, 2018

NOT TO BE MERGED: as this is dirty and quick poc, most of codes are duplicated between async interface

This PR aims to discuss viable path to #331, primarily to support synchronous interface to SourceMapConsumer relies on wasm binary. There are 2 major places async instantiation involved in SourceMapConsumer:

  • reading (or fetching) wasm binary as buffer
  • compile binary via WebAssembly.instantiate

In this changes, exposes new interface named SourceMapConsumerSync as non-major-breaking interface, and replaces above logic to sync via

  • Embed base64 encoded wasm binary, synchronously read it as buffer
    : Inspired by emscripten's SINGLE_FILE=1 option. Instead of resolving physical file (or remote resource for fetch) includes binary into javascript, allows to read it as arraybuffer synchronously.

  • Replace WebAssembly.instantiate to WebAssembly.Module (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/Module)
    : When bufferSource is already available, it is possible to synchronously instantiate wasm module via WebAssembly.Module, then create instance via WebAssembly.Instance with importobject.

To verify changes, convert-pasted async test cases into sync test cases as well.

Still, this brings number of unanswered q, reason I consider this as rather discussion than to-be-merged PR.

  • size concerns:
    dist/*.js will bundle wasm binary now, which increases bytesize. I believe Remove the bundled dist/ directory? #362 is way to go and consumer should be able to tree-shake with own bundler config, so this would not be major deal.
  • ergonomics for interfaces:
    SourceMapConsumerSync is just to make poc convinient.
  • code deduplication:
    Except initialization, most of codes are shared between async to sync interface implementations.
  • blocking module initialization:
    Binary size is fairly small (~60kb), but this is still sync behavior, not sure about impact especially on browser environment. Maybe only support node.js would be feasible since most usecases for sync interface comes from node.js tooling.
  • do we really want this?:
    Even if I composed this PR, I strongly believe async interface is way to go and if any tools can refactor usage of this module it should be recommended. Not sure about exposing sync interface as escape hatches, as consumer will going to use this interface and won't be easily deprecated.

Thoughts? Ideas? I'm totally ok to close this PR, but wanted to raise it for discussions especially lot of tools I use (i.e source-map-support) would like to benefit from faster wasm implementation.

@coveralls
Copy link

coveralls commented Oct 31, 2018

Pull Request Test Coverage Report for Build 548

  • 301 of 348 (86.49%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.1%) to 87.266%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/wasm-sync.js 34 48 70.83%
lib/source-map-consumer-sync.js 264 297 88.89%
Totals Coverage Status
Change from base Build 546: -1.1%
Covered Lines: 1125
Relevant Lines: 1259

💛 - Coveralls

@kwonoj
Copy link
Author

kwonoj commented Oct 31, 2018

Additional note: I am certain both (b64 encoded binary / sync init) is not-in-favor in general (and I agree too), but was hard to achieve real sync interface without those.

@puzrin
Copy link

puzrin commented Sep 2, 2020

  • When bufferSource is already available, it is possible to synchronously instantiate wasm module via WebAssembly.Module, then create instance via WebAssembly.Instance with importobject.

@kwonoj keep in mind, some browsers have size limits for sync wasm load. I had such issues when packed wasm to b64 for image resizer. Personally, i would recommend b64 + async load.

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