twilio / twilio-video.js

Twilio’s Programmable Video JavaScript SDK
https://www.twilio.com/docs/video/javascript
Other
571 stars 217 forks source link

AMD support #17

Closed jaridmargolin closed 7 years ago

jaridmargolin commented 8 years ago

I looked at the dist files and it appeared there was support for AMD modules, however, utilizing require.js to bundle and almond.js on the client, I recieve the following error: Uncaught Error: See almond README: incorrect module build, no module name which traces back to the following (dist/twilio-conversations.js line 26525):

define([], function() { return Conversations; });

Updating to the following seems to work for my use case:

define('twilio-conversations', function() { return Conversations; });

https://github.com/requirejs/almond/blob/master/README.md#incorrect-module-build-no-module-name provides additional information on the subject.


Are there tests in place to confirm AMD is working correctly? I can dig deeper here and try to submit a PR if we are in agreement that the current code is not working as expected.

markandrus commented 8 years ago

@jaridmargolin thanks for writing in!

Are there tests in place to confirm AMD is working correctly? I can dig deeper here and try to submit a PR if we are in agreement that the current code is not working as expected.

There are no tests in place to confirm AMD is working correctly, but we should definitely add such tests. We would also accept a PR to fix this.

jaridmargolin commented 8 years ago

Thanks for the remarkably fast response 😊

I'll have to dig into the build process a little to see if it is a simple fix. It also appears to be an issue with twilio-common.js.

jaridmargolin commented 8 years ago

Looks as simple as updating https://github.com/twilio/twilio-conversations.js/blob/master/src/twilio-conversations.js#L10

  1. What would the module name be (twilioConversations or twilio-conversations)?
  2. Would you be open to accepting a PR here without accompanying tests? I am not 100% what the best approach to testing the dist files would be.
markandrus commented 8 years ago

It also appears to be an issue with twilio-common.js.

I think you are right.

  1. What would the module name be (twilioConversations or twilio-conversations)?

"twilio-conversations" I think.

  1. Would you be open to accepting a PR here without accompanying tests? I am not 100% what the best approach to testing the dist files would be.

I think we should have some tests, esp. since we didn't get this right initially. We can look into this on our end, but probably wouldn't be able to get to it until next week. As far as testing the dist files, I think we'd want to add a new step to our gulpfile.js that depends on dist. It would need to execute some tests using require.js/almond.js to exercise the dist. Failing these tests should fail the build.

jaridmargolin commented 8 years ago

I think the issue may actually lie deeper than just naming the module. Still trying to wrap my head around the host of issues r.js has with UMD wrappers.

I was experimenting to see if the module had to be named (I use anonymous modules without a problem all of the time). Anyways, I came across the following code in the dist file:

    if (typeof define === 'function' && define['amd']) {
      define(function() { return lib$es6$promise$umd$ES6Promise; });
    } else if (typeof module !== 'undefined' && module['exports']) {
      module['exports'] = lib$es6$promise$umd$ES6Promise;
    } else if (typeof this !== 'undefined') {
      this['ES6Promise'] = lib$es6$promise$umd$ES6Promise;
    }

This wonderful definition is not named either and as result causes failures in environments where Promises are not supported.


I am actually wondering if UMD is the correct approach.... It may be easier / more reliable to create separate distributions for common/amd/global 🙁

jaridmargolin commented 8 years ago

Unfortunately I don't have too much more time to dig into this issue. Surprised noone else has mentioned it. Perhaps I am the only dev still using the now archaic require.js

ryan-rowland commented 8 years ago

@jaridmargolin I've tested a few ways myself and haven't been able to reproduce this error. Could you walk me through how you're bundling twilio-conversations.js with r.js and almond.js?

markandrus commented 7 years ago

Hey @jaridmargolin,

We've made a number of changes since this ticket was opened (removing twilio-common.js, renaming twilio-conversations.js to twilio-video.js). We also landed some UMD tests a while back. For example, see here, where we require twilio-video.js—granted we are loading the relative path to the dist/ build, but I expect it would be similar.

If you are still having issues, do you mind re-opening this ticket (or opening a new one)?

Thanks, Mark