deepgram / deepgram-js-sdk

Official JavaScript SDK for Deepgram's automated speech recognition APIs.
https://developers.deepgram.com
MIT License
127 stars 45 forks source link

isServer() Is Incorrect if Frontend Adds Global process Var #211

Closed seflless closed 6 months ago

seflless commented 6 months ago

What is the current behavior?

IsServer() is returning true when run in browser in some conditions. Some projects add a global “process” variable to attach process info like the environment info or env variable to hang environment variables off of.

isServer’s implementation checks if process defined and if it is returns true.

What's happening that seems wrong?

Because isServer() === true when running in browsers in this situation, then it runs the server version of Deepgram, this leads to errors related to creating a Websocket and running server code not browser style code

Steps to reproduce

To make it faster to diagnose the root problem. Tell us how can we reproduce the bug.

  1. Create a global variable called ‘process’ in a web app, then attempt to create a deepgram client connection const deepgram = createClient(apiKey); const newConnection = deepgram.listen.live({ model: 'nova-2', // model: "nova", interim_results: true, smart_format: true, endpointing: 500, });

Expected behavior

What would you expect to happen when following the steps above?

It should detect its in the browser, and run the browser version of the Websocket creation.

I’m not sure what the bullet proof way to detect if running in node, browser, or other environments (like edge functions and Cloudflare workers). In our case checking for the absence of a window global is a better way, but that might not be general. Note, window isn’t defined when run in a worker in browsers, so even checking for window isn’t the full solution, if you want to support running deepgram inside a browser worker.

Please tell us about your environment

We want to make sure the problem isn't specific to your operating system or programming language.

It’s not environment specific, reviewing the source code reveals the issue

Other information

Anything else we should know? (e.g. detailed explanation, stack-traces, related issues, suggestions how to fix, links for us to have context, eg. stack overflow, codepen, etc)

lukeocodes commented 6 months ago

What version are you running? In the released version, we no longer wrap the WebSocket class in isServer, so this shouldn't happen any more

seflless commented 6 months ago

I’m away from my computer so can’t check what it resolves to, but here’s my version pattern in package.json

"^3.0.0-beta.2"

lukeocodes commented 6 months ago

Yes you can now update to "3.0.0", this issue is resolved there.

seflless commented 6 months ago

Double checked, yep fixed for me. Looks like isServer's implementation didn't change though, hopefully the issue doesn't bite someone in some other code path. Thanks!