twilio / twilio-voice.js

Twilio's JavaScript Voice SDK
Other
53 stars 53 forks source link

[BUG] cannot import Device or Call #204

Open electrovir opened 1 year ago

electrovir commented 1 year ago

Code to reproduce the issue:

import {assert} from '@open-wc/testing';
import {Call, Device} from '@twilio/voice-sdk';

describe('Twilio Call and Device', () => {
    it('are defined', () => {
        assert.isDefined(Call);
        assert.isDefined(Device);
    });
});

Expected behavior:

I can run tests for my browser code that imports Twilio Call or Device using web-test-runner

Actual behavior:

Safari (WebKit), Firefox, and Chrome (Chromium) all throw various errors about not being able to import Call or Device:

Software versions:

electrovir commented 1 year ago

Originally I thought this was related to #55, but our actual app runtime (built with webpack currently) imports Device and Call without issue (ever since #55 was fixed). Webpack may be doing some kind of magic (or maybe one of our plugins) to make this work.

electrovir commented 1 year ago

I notice that web-test-runner is trying to the es5 files, which use require instead of import (which fails in the browser). I expected this to not be an issue since this package defines both module and main in its package.json, but it looks like the exports definition is actually overriding these so they (main and module) are not doing anything.

I see ./esm is defined in exports, so I tried using that:

import {assert} from '@open-wc/testing';
import {Call, Device} from '@twilio/voice-sdk/esm';

describe('Twilio Call and Device', () => {
    it('are defined', () => {
        assert.isDefined(Call);
        assert.isDefined(Device);
    });
});

And now I get "Importing binding name 'EventEmitter' is not found." errors.

electrovir commented 1 year ago

Importing from either of the dist files (dist/twilio.js and dist/twilio.min.js) doesn't help. (Plus, you can't import those without modifying node_modules/@twilio/voice-sdk/package.json anyway since the exports field prevents you from importing from dist.) They both use require anyway (so it seems to not actually be built-for-browser output?).

(Btw this is coming from an attempt at upgrading from the V1 api)

electrovir commented 1 year ago

Further errors after making hacky fixes:

It appears that that this Twilio package is simply not expected to be used in the web without a significant build process in front of it.

Workaround for now: detect when tests are being run and use mock Twilio objects in that case. We'll have to write the mock ourselves (since I don't see one in any of these docs?), which will be cumbersome but at least possible.

charliesantos commented 1 year ago

Thanks for submitting @electrovir . Can you please confirm that you're only seeing this when using the web-test-runner? Regarding the dist folder, those are meant to be loaded via a script tag

<script type="text/javascript" src="twilio.min.js"></script>

Once loaded, the SDK attaches a global Twilio object where you can access the Device class etc.

const Device = Twilio.Device;
const Call = Twilio.Call;
electrovir commented 1 year ago

It's only happening with web-test-runner and not webpack (which we currently use to bundle our frontend code), yes.

srtella commented 11 months ago

We are also facing similar issue with the voice device import statement (tried with voice sdk 2.8.0 ): import { Device } from "@twilio/voice-sdk/esm". We are using dev-server . When we try to run our application, we are getting error: /../../events/events.js' does not provide an export named 'EventEmitter' (at backoff.ts:8:10)

This is blocking us from updating the SDK to latest versions.

charliesantos commented 10 months ago

@electrovir @srtella we're going to start looking at this in the next couple of weeks. To speed up the process, do you have any example application that we can download and run to reproduce the issue without us having to build something from scratch?

electrovir commented 10 months ago

I can set that up for you.

charliesantos commented 10 months ago

@electrovir that would be great! Thank you.

electrovir commented 10 months ago

Here it is: https://github.com/electrovir/broken-twilio-device-import-example

Note that you can see an example test failure here: https://github.com/electrovir/broken-twilio-device-import-example/actions/runs/7064366321/job/19232242869#step:4:140

charliesantos commented 9 months ago

@electrovir @srtella looking at this more, it seems @web/test-runner is not properly handling CommonJS modules. Unlike webpack, even when using ESM, everything works fine. Does @web/test-runner have any config that stops treating CommonJS as ESM?

Anyway, we can try to update all our dependencies to use ESM but that would take a significant amount of time and effort. I don't see it happening soon. In the meantime, I see that @electrovir has a workaround. You can either:

charliesantos commented 9 months ago

@electrovir @srtella by the way, you have mentioned you observed this issue since you're upgrading to v2 of the voice sdk. Are you not seeing this on v1?