feat: Upgrade jitsi-meet-electron-sdk to ES Modules & fix #140

[AmoghParmar]

This PR request upgrades the repo 'jitsi-meet-electron-sdk' to use ES modules, and this will helps to improving organization code and also enabing performance through tree shaking.

The Changes are done:

Converted Common Js modules to ES modules : basically replaced 'require' with 'import' and 'module.exports' with 'export'.

Add 'type:module' in package.json : to indicate that project is now uses ES modules.

[saghul]

THank for your PR! Please drop the yarn lock file, we don't use yarn in this project.

[AmoghParmar]

I've removed the yarn.lock file and pushed the changes. Please let me know if any further changes are needed..!

[AmoghParmar]

For fixing linting error i renamed the eslintrc.js to eslintrc.cjs...

[saghul]

Please be patient with us, this is a big change and needs to be tested very carefully.

[AmoghParmar]

Hii @saghul ..I'm seeing the CI/Build win32-x64 and CI/Build win32-x86 checks failingg. Could you please provide some guidance on how to adress these issues? I'm not sure what's causing the failures

[csett86]

@AmoghParmar please see the inline comment on why the tests fail

[csett86]

The tests on win32 and win64 fail, because node-gyp-build needs some more tweaking than just an import, see https://www.npmjs.com/package/node-gyp-build for details.

And the tests are "just" doing that, ensuring that the native code import still works, @AmoghParmar

[csett86]

I have a fundamental question: Is this a fully breaking change, ie. all users would need to switch to the import syntax when using the SDK or is this backwards compatible?

Apart from that 2 inline questions, @AmoghParmar

[csett86]

Why is this file necessary in addition to the top-level binding.gyp?

[AmoghParmar]

Initially "node_addons/sourceId2Coordinates/binding.gyp" was created to adress a particular build config issue related to the sourceId2Coordinates. While solving the CI build issue , i believed it needed its own "binding.gyp" to isolate and customize its build settings. However after further investigation , i improvise the code that issue could be resolved by modifiyng the top-level binding.gyp without requiring a sepearate "binding.gyp" within the "node_addons/sourceId2Coordinates" directory. So thats why i removed the "binding.gyp".

[csett86]

Maybe a stupid question, but why is webpack now required if this is "just" converted to a ES module?

[AmoghParmar]

Maybe a stupid question, but why is webpack now required if this is "just" converted to a ES module?

As mentioned by @saghul in the issue #140 , The webpack is required for the renderer process. It optimizes and enables tree-shaking, for the browser environment. While ES modules handle import but webpack is necessary for browser specific build.

Please let me know if you have any further suggestions for improvementss..

[csett86]

This tree-shaking can only happen where the SDK is used, not within the SDK itself, as only then the final entrypoint is available and unused parts eg of this sdk can be omitted for the renderer

Eg here we currently exclude it from tree-shaking and optimizing: https://github.com/jitsi/jitsi-meet-electron/blob/master/webpack.renderer.js#L78

[csett86]

So with that I dont think webpack et al are required here in the SDK

[AmoghParmar]

Thank you, @csett86. The externals configuration in jitsi-meet-electron/webpack.renderer.js (externals: [ { '@jitsi/electron-sdk': 'require('@jitsi/electron-sdk')' } ]) prevents webpack from processing the SDK.

Therefore, webpack optimizations like tree-shaking are inapplicable to the SDK's code. Consequently, webpack configurations within the SDK are redundant and should be removed.

Your clarification is appreciated. Please provide any further guidance...!!

[csett86]

On that topic of tree-shaking: What I am also wondering: Does these entry-points for the screensharing tracker conflict with the whole ES module idea, as there files from the filesystem are loaded for screensharing

https://github.com/jitsi/jitsi-meet-electron-sdk/blob/master/screensharing/main.js#L182
https://github.com/jitsi/jitsi-meet-electron-sdk/blob/master/screensharing/main.js#L208

and for the alwaysontop window:

https://github.com/jitsi/jitsi-meet-electron-sdk/blob/master/alwaysontop/render/index.js#L201-L214

[AmoghParmar]

I have a fundamental question: Is this a fully breaking change, ie. all users would need to switch to the import syntax when using the SDK or is this backwards compatible?

Apart from that 2 inline questions, @AmoghParmar

Thankyou @csett86, for your review and raising these imp quest..

For Backward compatibility this change is intended to be a fullyy breaking change.. The SDK will require users to adopt the import syntax when working with ES modules.

I knw this has significant implications but i believe the long term benefits  regarding code org, tree-shaking which can make the code run faster, & alignment with modern JS practices justify the changess.

[csett86]

@AmoghParmar Making this a breaking change is a dealbreaker for me, we need to keep the commonjs around, so please investigate how to expose it both as a commonjs and ES module

[AmoghParmar]

@AmoghParmar Making this a breaking change is a dealbreaker for me, we need to keep the commonjs around, so please investigate how to expose it both as a commonjs and ES module

I agree with you @csett86 maintaining CommonJS compatibility is essential.

We need to find a solution that allows us to modernize the code while ensuring backward compatibility...

[saghul]

@csett86 If we bump the major, would this still be a dealbreaker?

I wouldn't be against maintainig a v7 branch for a little longer while users update to a new version 8. WDYT?

[csett86]

I would for now keep it compatible, and when we drop the legacy screensharing code (#434 (comment)) and anyway bump to v8, then we can also drop the commonJS.

How much effort do you see in providing both a commonjs and a ES module entrypoint, @AmoghParmar?

[saghul]

I have a hunch it would be a tall order. I may be wrong though!

Numbers are free, we can do a v9, etc.

Just an idea :-)

[csett86]

Looking at https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c, if we switch this to pure ES modules, then also all users of the SDK have to do that, if I am not mistaken. Maybe I am also on a wrong track here, so happy to get your input @saghul, @AmoghParmar

[saghul]

I'd need to double check, but IIRC it depends on what the app does. If it bundles the code with webpack for example it should Just Work (TM). We certainly need some clarity before we go breaking everything, for sure.

[AmoghParmar]

i found some technquies that maybe can help us to achieve our need for dual commonJS and ES modules entry point. I need some time to explore these techniquess..
@saghul, @csett86, if you have any further ideas on how we can best address this. Let us know, so we can align our approach with your vision

[csett86]

Just today the following post surfaced on hackernews, which seems more optimistic than what I linked before: https://antfu.me/posts/move-on-to-esm-only

Esp the require(esm) in nodejs should help us for the parts running in the main thread of electron. So from that perspective I was probably too cautious in my previous comments

[csett86]

But we still need to make sure that we bump the major before merging this

[saghul]

Thanks for keeping an open mind and for the extra reading resource!