lib-jitsi-meet icon indicating copy to clipboard operation
lib-jitsi-meet copied to clipboard

fix(package.json) Replace 'module' field with 'export' in package.json

Open michalsnik opened this issue 3 years ago • 8 comments
trafficstars

This PR updates package.json and replaces currently used 'module' field with a recommended by NodeJS 'export' field. According to documentation the 'module' field is ignored by NodeJS and only used by bundlers. After changing it however everything seems to be building fine.

The reasoning behind this change:

  • Currently when installing the package via npm using a link to a released archive, for example: https://github.com/jitsi/lib-jitsi-meet/releases/download/v1486.0.0+73bc6ba2/lib-jitsi-meet.tgz importing the lib throws a build error as it can't locate the module, given 'module' field being ignored by NodeJS and there is no index.js in the root directory.

With this change it's possible to import the package without any hiccups:

import JitsiMeetJS from 'lib-jitsi-meet';

I noticed that in jitsi-meet app you're importing the package like that already, but I'm not entirely sure why it works for you, perhaps you got some extra configuration that resolves modules by looking at that 'module' field 🤔

michalsnik avatar Aug 09 '22 06:08 michalsnik

Hi, thanks for your contribution! If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

jitsi-jenkins avatar Aug 09 '22 06:08 jitsi-jenkins

Have you verified the mobile app also builds ok?

saghul avatar Aug 09 '22 06:08 saghul

I just run the app on iOS and build via metro went just fine.

What's interesting though, it shouldn't build due to Metro not supporting "exports" fields yet (https://github.com/facebook/metro/issues/670), however conducting more experiments I noticed that metro actually takes the package from the "browser" field, not "modules". Even in log messages it seems like they treat "browser" as "main" field (if we specify wrong package in "browser" field - the error from metro tells that field "main" points to a non-existent package 🤷)

Btw. when you work on the lib, how do you link it to the mobile app locally? Metro doesn't seem to follow symlinks so can't do typical npm link ..., or am I missing something?

michalsnik avatar Aug 09 '22 07:08 michalsnik

Another option would be to add the react-native field to make sure RN prefers the ESM bundle, which is what it uses today.

Btw. when you work on the lib, how do you link it to the mobile app locally? Metro doesn't seem to follow symlinks so can't do typical npm link ..., or am I missing something?

You can use install-local for example.

saghul avatar Aug 09 '22 07:08 saghul

Replacing 'browser' with 'react-native' and pointing to ESM module works fine, though it throws one warning about circular dependency. Do you want me to fix it as part of this PR too?

I didn't get that warning using current version of the lib, so I'm thinking that RN currently uses the UMD version of the lib, not the ESM.

Simulator Screen Shot - iPod touch (7th generation) - 2022-08-09 at 23 26 44

michalsnik avatar Aug 09 '22 17:08 michalsnik

Hum, I was convinced we were using the ESM build, I may be wrong.

Yeah we should get the cycle fixed if that's the only one.

saghul avatar Aug 09 '22 17:08 saghul

I fixed the circular dependency in latest commit @saghul

michalsnik avatar Aug 10 '22 10:08 michalsnik

Should I change anything, or we can consider this fix complete? :)

michalsnik avatar Aug 29 '22 15:08 michalsnik