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

[EXPO 50] Super Expression must either be null or a function #1503

Closed
ScreamZ opened this issue Jan 29, 2024 · 34 comments · Fixed by #1515
Closed

[EXPO 50] Super Expression must either be null or a function #1503

ScreamZ opened this issue Jan 29, 2024 · 34 comments · Fixed by #1515
Labels
bug Something isn't working as expected confirmed The issue has been confirmed needs-reviewing A review is needed

Comments

@ScreamZ
Copy link

ScreamZ commented Jan 29, 2024

I'm using expo 50 dev client on Android

Whenever I write :

    import { mediaDevices } from "react-native-webrtc";

    const devices = await mediaDevices.getUserMedia({ video: true });

from MediaStreamTrackEvent.ts 18:43

@saghul
Copy link
Member

saghul commented Jan 29, 2024

Can you please share the whole backtrace?

@fobos531
Copy link

Can you please share the whole backtrace?

Hey @saghul , I do not have the entire backtrace, or a full repro, but I'm pretty certain this is, to at least some extent, linked to this issue: #1488

It seems like starting from RN 0.73, there's a mismatch in the expected version of event-target-shim. As you know, react-native-webrtc uses 6.0.2 of that dependency, but it seems like RN expects a different one (presumably 5.x), hence causing the error you see in the above issue. You can see additional discussion about this issue in the linked issue (#1488) as well as this one: GetStream/stream-video-js#1231 (I think GetStream uses a fork of react-native-webrtc). I tried all of the proposed solutions I came across so far, but ultimately I was unable to completely fix it, there was always some kind of hermes failure on launch whenever I tried consuming react-native-webrtc.

There's also a pending PR in expo/config-plugins#211 which also highlights this issue.

I hope this gives at least a little direction on what might be the root issue here.

@ScreamZ
Copy link
Author

ScreamZ commented Jan 29, 2024

IMG_4833.mp4

Its a bit complicated i had to take a video because my android device is embedded in a casing, and have no access to screenshot buttons ahah

@ScreamZ
Copy link
Author

ScreamZ commented Jan 29, 2024

I can confirm there are no issues with SDK 49

@saghul
Copy link
Member

saghul commented Jan 29, 2024

I don't get it, it should be possible to use different versions, just like any other Node package... We don't rely on the globals but import from the actual package...

Alternatively we could downgrade, but that will be a pain :-/

@fobos531
Copy link

I don't get it, it should be possible to use different versions, just like any other Node package... We don't rely on the globals but import from the actual package...

Alternatively we could downgrade, but that will be a pain :-/

Yup, even if you force Metro to explicitly use the one specified by react-native-webrtc systemwide, I haven't had success with that approach (while it worked for some users).

Do you think there would be tangible downsides to downgrading & keeping event-target-shim in sync with latest React Native?

@saghul
Copy link
Member

saghul commented Jan 29, 2024

No downside other than having to do the actual work.

@fobos531
Copy link

No downside other than having to do the actual work.

Hmm, okay. Yeah, I think it might be worth at least trying to downgrade to see if it will resolve this issue.

@fobos531
Copy link

fobos531 commented Jan 29, 2024

No downside other than having to do the actual work.

Hmm, okay. Yeah, I think it might be worth at least trying to downgrade to see if it will resolve this issue.

@saghul If you know off the top of the head the concrete steps that would need to be taken to do this, I can perhaps do a patch locally and test if it works, and if all works well, maybe I can submit a PR.

@saghul
Copy link
Member

saghul commented Jan 29, 2024

It's not a simple patch because the API changed.

@fobos531
Copy link

Hey @saghul

Check this GetStream/stream-video-js#1231 (comment) comment out for a potential workaround. While this does seem to work, I'm not sure whether it should be considered as a de-facto solution. With that said, it might still be a viable idea to align the API of react-native-webrtc to use v5 of event-target-shim

@saghul
Copy link
Member

saghul commented Jan 31, 2024

Yep, I'll work on the downgrade as soon as I have some spare cycles...

@8BallBomBom
Copy link
Member

8BallBomBom commented Jan 31, 2024

Honestly surprised how something simple is causing such an issue 🤔
Question is if we go through with the downgrade, how long will it be before we go through reverting again...

@8BallBomBom 8BallBomBom added bug Something isn't working as expected confirmed The issue has been confirmed needs-reviewing A review is needed labels Jan 31, 2024
@saghul
Copy link
Member

saghul commented Jan 31, 2024

Should be easy to revert since it'll be 1 commit I hope...

@8BallBomBom
Copy link
Member

Currently looking at reverting to a previous point, not exactly sure why we used defineCustomEventTarget 🤔
Should be a simple case of defining an event list for each thing, classes extending EventTarget with the event list and also defineEventAttribute for the onXXX functions to work correctly far as i can tell.

@ScreamZ
Copy link
Author

ScreamZ commented Feb 1, 2024

have you seen this partykit/partykit#516 that i tagged for ? Maybe could help

@saghul
Copy link
Member

saghul commented Feb 1, 2024

I don't think that's a good solution.

@8BallBomBom
Copy link
Member

8BallBomBom commented Feb 1, 2024

After looking over a few things i can understand the previous use of a custom event target but as for reverting, we could potentially do a better job but depending on how you look at it due to converting over to TypeScript and the existence of wonky polyfills, that could prove to be a bit of a pain 😨

@santhoshvai
Copy link
Contributor

this has been fixed in the expo plugin https://github.com/expo/config-plugins/blob/main/apps/react-native-webrtc/metro.config.js

@saghul
Copy link
Member

saghul commented Feb 7, 2024

Oh, nice! Has there been a release made?

@saghul
Copy link
Member

saghul commented Feb 7, 2024

Well hold on. Those not using Expo are going to run into this as well. I think downgrading is the sensible thing to do.

@santhoshvai
Copy link
Contributor

santhoshvai commented Feb 8, 2024

@saghul I wonder if the same solution can be applied to non expo users as well

if the metro config here is updated, that would influence which event-target-shim is used for this lib I guess as it works for expo.

@saghul
Copy link
Member

saghul commented Feb 8, 2024

Maybe that's an option indeed. Would you be able to submit a pr to that end?

@8BallBomBom
Copy link
Member

Would definitely be a viable solution vs reverting and using directive comments to ignore the compiler hell 😆

@wajeshubham
Copy link

I tried installing latest event-target-shim to my root project and it worked fine without breaking the app as it overrides default event-target-shim package installed while setting up the app. Still any update or workaround for this one without installing it explicitly?

@bogdankhamelyuk
Copy link

bogdankhamelyuk commented Feb 24, 2024

@wajeshubham that has helped me as well! Thx <3

@saghul
Copy link
Member

saghul commented Feb 25, 2024

I wonder if making it a peer dependency would help?

@8BallBomBom
Copy link
Member

Simplest of change but could help 🤔
It might kick out some warnings on install though.
Could be worth a try.

@saif-o99
Copy link

saif-o99 commented Mar 5, 2024

So what the last decision about this ? 😊

@feychenie
Copy link

FWIW Changing the import form import { Event, EventTarget } from "event-target-shim" to import { Event, EventTarget } from "event-target-shim/index" fixed the issue for me

@saghul
Copy link
Member

saghul commented Mar 7, 2024

Ha, interesting! Perhaps it's fooled by the fact that the name is technically different.

This looks like the simplest workaround!

@santhoshvai
Copy link
Contributor

santhoshvai commented Mar 7, 2024

this is such a nice workaround for this problem and a valid one!

I guess the reason why this works is because we specify a specific file for metro and not the package, and so metro is forced to look into the directory of the webrtc package to find the file.

@saghul
Copy link
Member

saghul commented Mar 7, 2024

I'd merge a PR with this workaround :-)

@8BallBomBom
Copy link
Member

Great find @feychenie 👍🏻
Will put up a quick pr, hopefully other projects eventually upgrade to a newer package 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected confirmed The issue has been confirmed needs-reviewing A review is needed
Projects
None yet
9 participants