webex / webex-js-sdk

JavaScript SDK for Webex
https://webex.github.io/webex-js-sdk/
Other
172 stars 340 forks source link

Webex SDK 3.0.0 not compatible with any version of NodeJS #3510

Open Tiuipuv opened 4 months ago

Tiuipuv commented 4 months ago

Describe the bug The following script fails to initialize in NodeJS:

import webex from 'webex';

webex.init({credentials: '<valid credentials were provided here>'});

Yielding the following error:

ReferenceError: window is not defined
    at Object.<anonymous> ([...]\node_modules\@webex\plugin-meetings\node_modules\@webex\internal-media-core\dist\cjs\index.js:2065:29024)
    at Module._compile (node:internal/modules/cjs/loader:1364:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1422:10)
    at Module.load (node:internal/modules/cjs/loader:1203:32)
    at Module._load (node:internal/modules/cjs/loader:1019:12)
    at Module.require (node:internal/modules/cjs/loader:1231:19)
    at require (node:internal/modules/helpers:177:18)
    at Object.<anonymous> ([...]\node_modules\@webex\plugin-meetings\dist\meetings\index.js:35:26)
    at Module._compile (node:internal/modules/cjs/loader:1364:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1422:10)

This has been tested with nodejs v12, v14, v16, v18, and v20. package.json:

{
  "name": "test_webex",
  "type": "module",
  [...]
  "dependencies": {
    "webex": "^3.0.0"
  }
}

This error is produced for both ESM modules and CommonJS scripts.

To Reproduce Steps to reproduce the behavior:

  1. create a blank npm project using npm init
  2. install webex sdk with npm install webex
  3. create index.js, with the above code (enter valid bot id)
  4. run with node index.js
  5. See error

Expected behavior Webex SDK successfully imports into script.

Platform (please complete the following information):

valgaze commented 4 months ago

+1 was about to file an issue as well

Confirm failure in node 21

image
valgaze commented 4 months ago

@Tiuipuv

Two things:

  1. This is happening because code modifies globalThis.navigator which in node 21, the navigator object is special
  1. Regardless the WebEx sdk is NOT isn't yet supporting newer versions of node: [Feb 2024] https://github.com/webex/webex-js-sdk/issues/3393#issuecomment-1959182736

Regarding item 1:

var commonjsGlobal = typeof globalThis !== 'undefined' ? globalThis : typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : {};

When unpacked it looks like this (and 1st condition matches in node21):

let commonjsGlobal;

if (typeof globalThis !== 'undefined') {
  commonjsGlobal = globalThis;
} else if (typeof window !== 'undefined') {
  commonjsGlobal = window;
} else if (typeof global !== 'undefined') {
  commonjsGlobal = global;
} else if (typeof self !== 'undefined') {
  commonjsGlobal = self;
} else {
  commonjsGlobal = {};
}

Later on this file mock some methods on a phony commonjsGlobal.navigator (which is ultimately globalThis.navigator):

commonjsGlobal.navigator = {
          userAgent: browserFakeUserAgent,
          getUserMedia: function getUserMedia() {}
      };

Regarding item 2: The lack of updates is a bummer, however, there might be fixes coming from node patches itself. The now-term solution is back down a version

cc: @arun3528 @jbenyovs

Tiuipuv commented 4 months ago

@valgaze

Thank you for your in depth analysis. However, I am not sure downgrading to any version of node works. I noticed this navigator error on v20+, but all versions below 20 all the way down to 12 we're also failing to startup as well (tested using volta).

When you proposed the 'now-term' solution of downgrading, were you referring to downgrading node, or the package itself back to 2.60.2?

valgaze commented 4 months ago

@Tiuipuv I'm realizing your error is slightly different:

Your issue: issues about window object in many versions of node as you stated

Other issue: globalThis.navigator only occurs in node 21+

They seem related but curious what the maintainers of this library think, but maybe it's smart to split this into separate issues

samtsai commented 4 months ago

2242 another issue exists for window is not defined

sreenara commented 4 months ago

@Tiuipuv thanks for reporting this issue. We have also observed this internally and are currently looking into this. Will provide updates once we have a chance to identify where the root cause is.

Just another note that at this time, we haven't tested all the features of the JS SDK with Node 18 and above. Our applications are running Node 16 and consuming the SDK.

sreenara commented 4 months ago

Tagging @ricardocasares as well from #2242

sreenara commented 4 months ago

Hey folks, I did some testing and here's a workaround you'll can use to continue testing. I tested this locally and was able to run it on Node 16, 18 and 20 to establish a successful registration with webex.meetings

global.window = {};
global.window.location = {};
global.window.navigator = {
    userAgent: 'test'
};
global.window.setTimeout = setTimeout;
// global.Webex = {};

const Webex = require('webex');

const webex = (window.webex = Webex.init({
    credentials: {
      access_token: 'token'
    }
  }));

webex.once('ready', () => {
    if (webex.canAuthorize) {
        console.log('webex is authorized');
        webex.meetings.register().then(() => {console.log('registered to meetings', webex.internal.device.userId);})
    }
});

This requirement of window object seems to have come from one of the dependencies and we are investigating it. It will take us a few days to get to the root cause.

arun3528 commented 3 months ago

@sreenara there is a need to discuss about how to ship non browser package efficiently + testing

Tiuipuv commented 3 months ago

Thank you for the quick movement on this issue. Looking forward to a solution. The above workaround, with setting global.window.* to browser simulated values, is a great workaround for a low dependency project. However, this is dangerous to do in an environment with tons of other packages that use the existence of certain global.window.* to know if they are running in the browser.

sreenara commented 3 months ago

Marking this as a feature and not a bug. On closer inspection of the code, what we are observing here is expected behavior. The goal now would be to find out whether we can publish a variant of webex.min.js that will NOT require the window object. In this variant,

enyineer commented 3 weeks ago

Is this still worked on? No fix since 3 months. I wouldn't have believed a commercial product to be so badly maintained. Suggesting to use EOL versions of Node just to use an SDK is not something I would expect from Cisco. Huge Red Flags to avoid this product and rather use competitor products...

sreenara commented 2 weeks ago

Is this still worked on? No fix since 3 months. I wouldn't have believed a commercial product to be so badly maintained. Suggesting to use EOL versions of Node just to use an SDK is not something I would expect from Cisco. Huge Red Flags to avoid this product and rather use competitor products...

@enyineer yes, we are planning to add support for NodeJS. The team will get to this in a month or so.

Just another note that at this time, we haven't tested all the features of the JS SDK with Node 18 and above. Our applications are running Node 16 and consuming the SDK.

Suggesting to use EOL versions of Node just to use an SDK

The Webex JS SDK can be compiled with Node 20 without any issues. We've also done some testing on Node 20. Meeting and calling features will work if the SDK is consumed on the client side.

johnwfrancis commented 2 weeks ago

@sreenara until it's fixed I've unpublished our node.js doc since the instructions don't actually work.