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

if (typeof fetch === "function") misidentifies node environment #349

Closed
sprockow opened this issue Jun 24, 2018 · 11 comments
Closed

if (typeof fetch === "function") misidentifies node environment #349

sprockow opened this issue Jun 24, 2018 · 11 comments

Comments

@sprockow
Copy link
Contributor

I am currently working building a node endpoint that accepts a minified stack trace from a client, and then uses this library to find the originating line in the un-minified source using a shared copy of the sourcemap.

The problem I'm running into is that the library uses "fetch" feature detection to determine whether the current environment is a browser or node environment.

if (typeof fetch === "function") {

and then conditionally exports a module in the read-wasm file: https://github.com/mozilla/source-map/blob/master/lib/read-wasm.js#L1

The node environment I'm developing within has been configured to use a isomorphic fetch polyfill, so this assumption has caused my node environment to incorrectly use the browser behavior and immediately throw the error:

You must provide the URL of lib/mappings.wasm by calling SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) before using SourceMapConsumer

I understand that my environment might be a bit different than most, but I'd like to challenge whether making assumptions like this is the correct way of browser vs. node detection. I came across a clever
solution described within this stackoverflow question, https://stackoverflow.com/questions/17575790/environment-detection-node-js-or-browser , where you can look up the default top-level function scope of the environment (window vs global). By looking at the default top level scope, this solution will even handle the case where window has been mocked out by a library like jsdom.

I've implemented this in a fork, and it seems to have fixed my problem. Wondering whether this is something you would be open to considering.

Also, if you otherwise think I've done something else completely wrong, please let me know. :)

@tromey
Copy link
Contributor

tromey commented Jun 27, 2018

I've implemented this in a fork, and it seems to have fixed my problem. Wondering whether this is something you would be open to considering.

It seems reasonable to me.

@sprockow
Copy link
Contributor Author

Ok, let me put together a pull request and I'll send this over for review.

@sprockow
Copy link
Contributor Author

sprockow commented Jul 11, 2018

Finally got around to doing this. Submitted #350 as a possible solution to this problem.

@tromey
Copy link
Contributor

tromey commented Aug 10, 2018

Fixed by #350.

@theKashey
Copy link

This issue is not solved or closed while fixed only in the beta version.

@loganfsmyth
Copy link
Contributor

@theKashey I don't know when we'll be doing further releases, but there should be no problem using the beta release if it's a feature that is important for your usecase.

@theKashey
Copy link

Unfortunately some people (or their security department) could not afford beta releases. But if beta.0 is actually stable - could you publish it as, for example, next tag?

jacobg213 added a commit to jacobg213/get-stack-trace that referenced this issue Oct 3, 2019
Firstly thanks very much for this excellent library.

# Problem
I am running a Nodejs app and am hitting the following error when using this library:
```
You must provide the URL of lib/mappings.wasm by calling SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) before using SourceMapConsumer
```

# Diagnosis
* A dependency of yours "source-map" detects the runtime (Nodejs vs browser) in a broken way. That library then throws an exception if some client-side-specific code is not run.
-*The "check" checks for the presence of a variable "fetch" and the reasonable thinking is that only browsers will have fetch. The issue is that in my case and in some others (see [here](mozilla/source-map#349)) 3rd party libraries can define a variable "fetch" which

# Tests
None
jacobg213 added a commit to jacobg213/get-stack-trace that referenced this issue Oct 3, 2019
Firstly thanks very much for this excellent library.

I am running a Nodejs app and am hitting the following error when using this library:
```
You must provide the URL of lib/mappings.wasm by calling SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) before using SourceMapConsumer
```

* A dependency of yours "source-map" detects the runtime (Nodejs vs browser) in a broken way. That library then throws an exception if some client-side-specific code is not run.
* The "check" checks for the presence of a variable "fetch" and the reasonable thinking is that only browsers will have fetch. The issue is that in my case and in some others (see [here](mozilla/source-map#349)) 3rd party libraries can define a variable "fetch" which triggers the described error.

None
@karlhorky
Copy link

karlhorky commented Oct 18, 2019

I'm also uncomfortable with using a beta version, given that it's been a year since the beta version has been released (will there ever be an 0.8.0 version with this same code?).

There is also the points that @Rich-Harris made in #400, which seem to be good reasons not to use the approach used in the beta.

This issue creates errors of non-insignificant complexity that can be difficult or impossible to debug, based on your skill level:

developit/unfetch#122

Of course, workarounds exist:

...but it seems like this should be fixed here in a non-beta version of source-map.

@loganfsmyth @fitzgen @tromey Would the team be open for considering another approach and releasing a non-beta 0.8.0 version?

cc @kunstloch

@simianarmy
Copy link

This is also an issue with package dependencies, for example we use react-hot-loader which has source-map as a dependency. I had to use yarn's resolutions feature to force my package manager to make react-hot-loader use the beta version - an awkward solution considering we don't use yarn

@theKashey
Copy link

@simianarmy - just don't use react-hot-loader/webpack loader for your files. That's not required, and not a good idea.

@simianarmy
Copy link

@theKashey I don't use that. Just using react-static

ascott18 added a commit to IntelliTect/Coalesce that referenced this issue Feb 17, 2023
Georift added a commit to Georift/source-map-to-source that referenced this issue Apr 12, 2024
`source-map` was incorrectly attempting to detect if we're in a browsers
context by checking if `typeof fetch === 'function'` which in never versions of
node if present. Because of this source-map was reqesting that we provide a
WASM file.

[read more here](mozilla/source-map#349)

Bumping to 0.8.0-beta.0 fixes this.

Also filters out any files in the sourcemap which don't have a file contents
associated with it.
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

No branches or pull requests

6 participants