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

[FEATURE] Avoid using nodejs modules #55

Closed
bluwy opened this issue Dec 16, 2021 · 32 comments
Closed

[FEATURE] Avoid using nodejs modules #55

bluwy opened this issue Dec 16, 2021 · 32 comments
Labels
enhancement New feature or request jira added

Comments

@bluwy
Copy link

bluwy commented Dec 16, 2021

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 sveltejs/kit#2548. Related #37. May be related #52

@cbschuld
Copy link

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

@ryan-rowland
Copy link
Contributor

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

@ankhuve
Copy link

ankhuve commented Mar 24, 2022

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.

image

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!

@DavidKvas
Copy link

Any ETA for this one?

@Jesusfco
Copy link

Jesusfco commented Jun 1, 2022

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
Copy link

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.

image

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
Copy link

alexvdvalk commented Jun 29, 2022

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
Copy link

cymen commented Aug 12, 2022

@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
Copy link

cymen commented Aug 13, 2022

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
Copy link

cymen commented Aug 13, 2022

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

@bld010
Copy link

bld010 commented Aug 17, 2022

cc: @mhuynh5757

@waynebrantley
Copy link

@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 [email protected]: 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
Copy link
Collaborator

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
Copy link

herosimo commented Nov 9, 2022

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
Copy link

@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/[email protected]

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
Copy link

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

@waynebrantley
Copy link

We need help Twilio - there are many tickets about these kinds of issues.
#76
#72
#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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

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

@charliesantos
Copy link
Collaborator

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
Copy link

@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
Copy link

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
Copy link

jordanful commented Apr 26, 2023

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

@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
Copy link

@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
Copy link

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/[email protected]/dist/twilio.min.js

@charliesantos
Copy link
Collaborator

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
Copy link

@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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request jira added
Projects
None yet
Development

No branches or pull requests