Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

michalsnik
Copy link

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:

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 🤔

@jitsi-jenkins
Copy link

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 :(.

@saghul
Copy link
Member

saghul commented Aug 9, 2022

Have you verified the mobile app also builds ok?

@michalsnik
Copy link
Author

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 (facebook/metro#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?

@saghul
Copy link
Member

saghul commented Aug 9, 2022

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.

@michalsnik
Copy link
Author

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

@saghul
Copy link
Member

saghul commented Aug 9, 2022

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.

@@ -23,7 +23,7 @@
*
* @constructor
*/
export default function TraceablePeerConnection(rtc: RTC, id: number, signalingLayer: any, pcConfig: object, constraints: object, isP2P: boolean, options: {
export default function TraceablePeerConnection(rtc: any, id: number, signalingLayer: any, pcConfig: object, constraints: object, isP2P: boolean, options: {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing RTC import from TraceablePeerConnection.js results in missing type here.. not sure if we can do something about that right now, thoughts?

@michalsnik
Copy link
Author

I fixed the circular dependency in latest commit @saghul

@michalsnik
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants