[FEATURE] Avoid using nodejs modules

bluwy commented 2 years ago

Is your feature request related to a problem? Please describe. The library is using the util module, which is a module from nodejs. Recent bundlers like Vite and Webpack 5 don't shim nodejs modules anymore, so this would fail to bundle for them.

Describe the solution you'd like Refactor the code to avoid the util module, maybe with a custom implementation of the inherits function specifically, since that's the only utility used.

Describe alternatives you've considered Document that this library does not work without shimming nodejs modules.

Additional context Initially reported at https://github.com/sveltejs/kit/issues/2548. Related #37. May be related #52

cbschuld commented 2 years ago

Also related to #28 (anything with Webpack >=5)

ryan-rowland commented 2 years ago

Thank you for surfacing this, I have added a ticket for this internally and intend to start investigating sometime this week.

ankhuve commented 2 years ago

We're experiencing problems with v2.1.1, which I assume stems from the same issue. We recently migrated from CRA to Vite, and since then we're not able to run Device.prototype.connect (much like #52 ).

After setting the log level to debug and following the logs, I can see that https://github.com/twilio/twilio-voice.js/blob/master/lib/twilio/wstransport.ts#L8 resolves the import of WebSocket through @twilio/voice-sdk/node_modules/ws/browser.js, which just throws the error ws does not work in the browser. Browser clients must use the native WebSocket object.

I've experienced similar issues with other libraries (https://github.com/aws-amplify/amplify-js/issues/7499#issuecomment-804386820 as well as https://github.com/contentful/contentful.js/issues/422) which requires us to insert the following into our index.html:

<script>
    // Need this for aws-amplify to not throw errors
    // https://github.com/aws-amplify/amplify-js/issues/7499#issuecomment-804386820
    const isBrowser = () => typeof window !== 'undefined';
    const isGlobal = () => typeof global !== 'undefined';
    var exports = {};
    if (!isGlobal() && isBrowser()) {
        var global = window;
        var process = process || {
            env: { DEBUG: undefined },
            version: [],
            // browser property added because Contentful thinks it's running in a node environment otherwise
            browser: true,
        };
        var inherits = {};
    }
</script>

I don't think any of that should affect the behavior of Twilio though, just including my experience of these types of issues in case it matters somehow.

What's interesting is that the issue only occurs when we run Vite in development/watch mode - when we build and serve the code, everything works as expected.

Would be great to get your thoughts on this and how we can get this to work in development as well!

DavidKvas commented 2 years ago

Any ETA for this one?

Jesusfco commented 2 years ago

We where migrating from wepack to vite to work vue project and all works on production but on local development this function not broke but not return anything

Device.prototype.connect

asanchezGL commented 2 years ago

We're experiencing problems with v2.1.1, which I assume stems from the same issue. We recently migrated from CRA to Vite, and since then we're not able to run Device.prototype.connect (much like #52 ).

After setting the log level to debug and following the logs, I can see that https://github.com/twilio/twilio-voice.js/blob/master/lib/twilio/wstransport.ts#L8 resolves the import of WebSocket through @twilio/voice-sdk/node_modules/ws/browser.js, which just throws the error ws does not work in the browser. Browser clients must use the native WebSocket object.

I've experienced similar issues with other libraries (https://github.com/aws-amplify/amplify-js/issues/7499#issuecomment-804386820 as well as contentful/contentful.js#422) which requires us to insert the following into our index.html:
<script>
    // Need this for aws-amplify to not throw errors
    // https://github.com/aws-amplify/amplify-js/issues/7499#issuecomment-804386820
    const isBrowser = () => typeof window !== 'undefined';
    const isGlobal = () => typeof global !== 'undefined';
    var exports = {};
    if (!isGlobal() && isBrowser()) {
        var global = window;
        var process = process || {
            env: { DEBUG: undefined },
            version: [],
            // browser property added because Contentful thinks it's running in a node environment otherwise
            browser: true,
        };
        var inherits = {};
    }
</script>
I don't think any of that should affect the behavior of Twilio though, just including my experience of these types of issues in case it matters somehow.

What's interesting is that the issue only occurs when we run Vite in development/watch mode - when we build and serve the code, everything works as expected.

Would be great to get your thoughts on this and how we can get this to work in development as well!

I'm having this issue. Is there any progress? It does not work with vitejs

alexvdvalk commented 2 years ago

I solved this by dynamically importing the minified js. Not the most elegant solution but it works for now. I'm using SvelteKit.


  const intitializeDevice = async () => {
    await import("@twilio/voice-sdk/dist/twilio.min.js");
    const Device = window["Twilio"].Device;
}

cymen commented 2 years ago

@alexvdvalk I think your code sample got cut off however it was very helpful -- I ended up doing this as the (hopefully temporary) workaround:

let Device;
(async () => {
  await import("@twilio/voice-sdk/dist/twilio.min.js");
  Device = window["Twilio"].Device;
})();

Then later waiting for Device to be defined before calling new on it.

cymen commented 2 years ago

I had build problems with the above so I ended up rolling back our vite version for now (from v3 to v2.9).

I'm really surprised at the lack of maintenance of this module by Twilio. It's very disappointing.

cymen commented 2 years ago

@ryan-rowland Did anything come out of the internal ticket?

bld010 commented 2 years ago

cc: @mhuynh5757

waynebrantley commented 1 year ago

@ryan-rowland @mhuynh5757 Is anything going to be done to move this library forward? It cannot be used by ESM modules as it is dependent on nodejs items that do not work in the browser. The way this is packaged just needs cleaned up.

In addition there are super old dependencies such as:

@twilio/voice-sdk 2.1.1 └─┬ @twilio/audioplayer 1.0.6 └─┬ babel-runtime 6.26.0 └── core-js 2.6.12

WARN deprecated core-js@2.6.12: core-js@<3.23.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Some versions have web compatibility issues. Please, upgrade your dependencies to the actual version of core-js.

For those of us using this package in the browser to make/receive phone calls this needs a big cleanup/overhaul. Please advise.

charliesantos commented 1 year ago

Hello everyone. Apologies for the delay in response. There is plan to modernize this SDK but there is no timeline yet due to other higher priority items.

herosimo commented 1 year ago

I use Vite 3, React + TS.

The solution that works for me is to add Node.js polyfill to Vite.

First, install these packages: npm i @esbuild-plugins/node-globals-polyfill @esbuild-plugins/node-modules-polyfill --save-dev

Then add the polyfill config to your vite.config.js. Here is link of the code: https://medium.com/@ftaioli/using-node-js-builtin-modules-with-vite-6194737c2cd2.

Because I use React, I still need to add plugins: [react()] inside the polyfill config. So it becomes something like: export default defineConfig({ plugins: [react()], // rest of the polyfill config })

Now you can do import { Device } from "@twilio/voice-sdk"; and construct new Device(token).

waynebrantley commented 1 year ago

@herosimo Nice! Thanks for the information. What about bundle size? @esbuild-plugins/node-modules-polyfill adds 162kb minifed (49.5kb gzipped) https://bundlephobia.com/package/@esbuild-plugins/node-modules-polyfill@0.1.4

Did you experience a growth like that?

Twilio (a developer focused company) seems to show little interest in what developers need and to produce a high quality standards based client library. @charliesantos is there any timeline?

herosimo commented 1 year ago

@waynebrantley I didn't check the bundle size. Just looking temporary solution that works.

waynebrantley commented 1 year ago

We need help Twilio - there are many tickets about these kinds of issues. https://github.com/twilio/twilio-voice.js/issues/76 https://github.com/twilio/twilio-voice.js/issues/72 https://github.com/twilio/twilio-voice.js/issues/101

So while you have internal tickets, there appears to be no progress. Please advise if we will get some much need relief or if we should explore other options.

kbagchiGWC commented 1 year ago

Hi @waynebrantley

Apologies for the inconvenience. We plan to modernize this SDK and prioritizing discussion is in progress. We will get back to you as soon as the decision is made.

Thanks for being patient with us and again apologies for the delay.

charliesantos commented 1 year ago

Hello everyone, thank you all for providing feedback regarding this issue. In the meantime, can you please try @alexvdvalk 's workaround?

I solved this by dynamically importing the minified js. Not the most elegant solution but it works for now. I'm using SvelteKit.

const intitializeDevice = async () => { await import("@twilio/voice-sdk/dist/twilio.min.js"); const Device = window["Twilio"].Device;

You can also download the minified versions from this repo which you can load using the above method, or put in a script tag if possible. https://github.com/twilio/twilio-voice.js/tags

waynebrantley commented 1 year ago

Yes we are making that dynamic load work. However @charliesantos as a developer centric company this should be addressed long ago.

charliesantos commented 1 year ago

I completely agree @waynebrantley . We definitely understand your pain. As @kbagchiGWC mentioned, there is a lot of discussion regarding modernization of the SDK. Please stay tuned.

AxelTheGerman commented 1 year ago

@charliesantos @kbagchiGWC I understand the tough position that you're in, but this ticket has been open for over a year. About 10 months later you mentioned "plan to modernize this SDK". Another 3 months later there are still discussions and apparently higher priority items.

I don't think most folks chose Twilio for the competitive pricing but for the developer experience - I just spent 6 hours today trying to implement in browser calling and couldn't figure out why this isn't working until I finally found this ticket.

I really expected more from Twilio here, how can you ship the "next version of Twilio's Javascript Client SDK" that relies on NodeJS and doesn't work in the browser

hawkinbj commented 1 year ago

TL;DR we've escalated the broader issue (not just this specific one) and prioritized for early Q2; it is our immediate next-up batch of work.

@AxelTheGerman (and all others in this thread) - I'm the PM responsible for the Voice SDK suite, and therefore responsible for the prioritization decisions our teams have made. I won't bore you with the myriad reasons behind those priority calls because they're basically excuses and ultimately you're right - we've failed the developer community here. We've adjusted our internal roadmap to prioritize this as our immediate next working item. To set expectations, that probably means we're looking at Q2 to get a wholistic solution out given the repackaging/retooling required. I realize that might be too long of a wait for some customers, and understand that you'll need to make the best decision for you/your company/your customers.

Thank you for the feedback - my apologies it's taken so long to get the attention it deserves. We haven't forgotten about this community and what makes Twilio special. We're committed to fixing this and look forward to getting something into your hands soon.

jordanful commented 1 year ago

Chiming in to say I burned two entire days on this before finding this thread. I had to totally hack our Rails 7 app to get .connect to work, thanks to the help in this thread. Embarrassing for Twilio that one of your core SDKs just flat out hasn't worked for months.

For anyone working with import maps and Rails 7:

Copy twilio.min.js into public/javascripts directory
Pin it in your importmap.rb file: pin "twilio.min.js", to: "assets/javascripts/twilio.min.js"

If you are using Stimulus, add a function similar to the one below to your Stimulus controller.

async loadTwilioDevice() {
await import("/javascripts/twilio.min.js");
return window["Twilio"].Device;
}

Now you can use Device and call .connect on it without any issue, e.g .const Device = await this.loadTwilioDevice(); and then Device.connect. Hope this helps someone save 2 days of their time. Yeesh.

charliesantos commented 1 year ago

Hi @jordanful , sorry to hear about your troubles. This issue has been prioritized and will be worked on very soon. Just to make sure the work that we're doing fixes your issue, were your troubles also related to our use of nodejs util? Can you please provide errors/logs if the issue you're getting is different than what was already mentioned in this ticket?

jordanful commented 1 year ago

Just to make sure the work that we're doing fixes your issue, were your troubles also related to our use of nodejs util? Can you please provide errors/logs if the issue you're getting is different than what was already mentioned in this ticket?

I didn't see any errors, despite attempting to log out as much as possible. Perhaps I should have had debug mode on, as another commenter mentioned. Let me know how/if I can provide any more info.

charliesantos commented 1 year ago

@jordanful thanks for the quick response. I assume that your workaround is to use the minified js artifact because you're having issue using the npm module. Please correct me if my assumption is incorrect. Is the issue happening during build time or runtime? If during build time, please provide any terminal logs. If during runtime, please enable debug mode and provide console logs.

AxelTheGerman commented 1 year ago

@jordanful thanks for sharing your workaround.

Copy twilio.min.js into public/javascripts directory

Which minified JS file did you copy? Probably the browser SDK? I haven't touched any of this in a couple of months

jordanful commented 1 year ago

Which minified JS file did you copy? Probably the browser SDK? I haven't touched any of this in a couple of months

@AxelTheGerman this guy: https://unpkg.com/@twilio/voice-sdk@2.4.0/dist/twilio.min.js

charliesantos commented 1 year ago

Hey everyone, this issue should be fixed in the upcoming release (version 2.6.0). We have an RC that is currently being validated and will be promoted to GA once it's ready. We performed some initial testing on Svelte+Vite and Angular 16 and everything is working as expected.

In the meantime, you can verify if it works in your environment by installing the RC version from a github tag.

npm install twilio/twilio-voice.js#2.6.0-rc1

Changes in this release include:

Removed usage of NodeJS modules from the SDK and some dependencies. With this change, the SDK should now work with some of the latest frameworks that use the latest versions of bundlers such as Vite and Webpack.
Removed unnecessary files from the generated npm package.
Links to source maps are now included in the generated npm package.

Thank you for your contributions and for bearing with us. I will close this ticket now since the original issue has been addressed. Please feel free to follow up if you have any questions.

waynebrantley commented 1 year ago

@charliesantos Previously, to work around all these issues, I did a dynamic load of the minimized file directly. I would add @twilio/voice-sdk version 2.5 in package.json and then use code like this

const intitializeDevice = async () => {
   await import("@twilio/voice-sdk/dist/twilio.min.js")
    Device = window["Twilio"].Device
}
intitializeDevice()

This correctly resulted in a split file added to my build like this: dist/assets/twilio.min-ab55cede.js 254.40 kB │ gzip: 71.72 kB │ map: 657.15 kB

Now that I have used the new RC above, it is of course just included directly in my bundle (which is fine or I can split of course) My new bundle size decreased around 20k from what you see above, which is a nice benefit.

charliesantos commented 1 year ago

We just released 2.6.0 which contains the fix for this issue.

twilio / twilio-voice.js

[FEATURE] Avoid using nodejs modules #55