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

Split up wasm loading based on compilation target. #363

Merged
merged 1 commit into from
Nov 13, 2018

Conversation

loganfsmyth
Copy link
Contributor

Per my comment in #350 (comment), this feels like a more ideal way to approach this. This leaves it up to bundlers to swap out the files when bundling for browser use.

I'm personally a fan of this approach because it's also an easily-reproducible check. As it is, there's no way for a module making use of source-map to know whether a call to SourceMapConsumer.initialize is needed, or will result in a console.debug("SourceMapConsumer.initialize is a no-op when running in node.js"); message. This gives one central toggle for that, which they can also use in their own code when deciding whether to call .initialize or not.

@coveralls
Copy link

coveralls commented Oct 10, 2018

Pull Request Test Coverage Report for Build 530

  • 9 of 12 (75.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 88.875%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/read-wasm.js 9 12 75.0%
Totals Coverage Status
Change from base Build 528: 0.8%
Covered Lines: 873
Relevant Lines: 960

💛 - Coveralls

@tromey
Copy link
Contributor

tromey commented Oct 16, 2018

This seems better to me as well.

I tend to think it should land after the patch to remove dist, so not r+'ing it yet; let me know what you think. Also it needs to update CHANGELOG.md.

@loganfsmyth
Copy link
Contributor Author

Yup, I just posted everything I had in mind for now, but agreed that landing it after dist makes sense.

@loganfsmyth loganfsmyth merged commit 76be1e6 into master Nov 13, 2018
@loganfsmyth loganfsmyth deleted the browser-build branch November 13, 2018 22:44
@AdrianMrn
Copy link

AdrianMrn commented Mar 26, 2019

I can't figure out how to use this package in a browser. I'm using webpack with target: 'web' and I try setting the location of mappings.wasm, but I get an error Can't resolve 'fs' in read-wasm.js.

I get the error the second I import the package, so it's not even reaching the code where I set the url.

import sourceMap from 'source-map';

sourceMap.SourceMapConsumer.initialize({
    "lib/mappings.wasm": "xxx/mappings.wasm"
});

@loganfsmyth
Copy link
Contributor Author

@AdrianMrn Have you potentially overridden the default mainFields value in Webpack? https://webpack.js.org/configuration/resolve#resolvemainfields As long as your target is web the read-wasm.js file shouldn't even be loaded:

"./lib/read-wasm.js": "./lib/read-wasm-browser.js"
If you aren't using browser as a mainFields value that would explain the issue.

@AdrianMrn
Copy link

AdrianMrn commented Mar 24, 2020

Excuse the necro. I just picked this back up and managed to fix my issue.

It seems that, when I pulled in the package through npm, it didn't get me the latest version that has the line you mentioned, because it's a beta release. I had to manually set my version of source-map to 0.8.0-beta.0 for it to work.

@yf-yang
Copy link

yf-yang commented Feb 7, 2023

I encountered the same issue as AdrianMrn stated, then found I was using 0.7.4. Since 0.8.0-beta.0 has been released for 4 years without updates, is it possible to merge this commit into 0.7.x?

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.

5 participants