Skip to content

Add option to initialize mappings wasm via ArrayBuffer - #339

Merged
fitzgen merged 2 commits into
mozilla:masterfrom
rtsao:wasm-init-arraybuffer
May 29, 2018
Merged

Add option to initialize mappings wasm via ArrayBuffer#339
fitzgen merged 2 commits into
mozilla:masterfrom
rtsao:wasm-init-arraybuffer

Conversation

@rtsao

@rtsaortsao commented May 22, 2018

Copy link
Copy Markdown
Contributor

This PR adds the ability to directly pass in the mappings wasm into SourceMapConsumer.initialize. This is needed for browser usage without needing to make a request (e.g. needing to work around CSP restrictions).

Example

constcontents=/* the ArrayBuffer contents of mappings.wasm */sourceMap.SourceMapConsumer.initialize({"lib/mappings.wasm": contents});

I didn't implement this because it would be a breaking change, but I think a better alternative API might be something like:

constwasmUrl="https://unpkg.com/source-map@0.7.3/lib/mappings.wasm";sourceMap.SourceMapConsumer.initialize({"lib/mappings.wasm": fetch(wasmUrl).then(res=>res.arrayBuffer())});

@coveralls

coveralls commented May 22, 2018

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 491

  • 0 of 8(0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 83.919%

Changes Missing CoverageCovered LinesChanged/Added Lines%
lib/read-wasm.js080.0%
TotalsCoverage Status
Change from base Build 490:-0.2%
Covered Lines:908
Relevant Lines:1051

💛 - Coveralls

@fitzgenfitzgen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@fitzgenfitzgen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh wait -- before we merge this, can you add a small test for initializing with an array buffer? Thanks!

@rtsao

Copy link
Copy Markdown
ContributorAuthor

I was going to, but with the existing test tooling, there's no way to write a test for this because sourceMap.SourceMapConsumer.initialize is a no-op in a Node.js environment. In fact, currently, it appears all browser-specific logic is entirely untested.

Do you have an opinion regarding how browser testing should be done?

@fitzgen

Copy link
Copy Markdown
Contributor

Bleh, I overlooked that. We would need some kind of headless browser testing setup. If you want to dig into adding that, please be my guest :)

In the meantime, we can just merge this. Thanks again!

@fitzgen

Copy link
Copy Markdown
Contributor

Can you rebase this branch on master?

@rtsao
rtsaoforce-pushed the wasm-init-arraybuffer branch from 7cf7e82 to 6e618f5CompareMay 29, 2018 18:22
@rtsao

Copy link
Copy Markdown
ContributorAuthor

Just did, thanks! 😄

Sign up for freeto 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.

3 participants

@rtsao@coveralls@fitzgen