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

Questionable addEventListener "fix" #458

Closed
awkay opened this issue Dec 29, 2024 · 8 comments
Closed

Questionable addEventListener "fix" #458

awkay opened this issue Dec 29, 2024 · 8 comments
Assignees
Labels
Milestone

Comments

@awkay
Copy link

awkay commented Dec 29, 2024

I was having the problem with addEventListener (electron/node) that this latest version fixes this way:

#?(:cljs
   (when-not node-target?
     (.addEventListener goog/global "beforeunload"
       (fn [event] (reset! client-unloading?_ true) nil))))

but I also use Sente in RN, and I'm pretty sure it will fail there too (unverified). Why not just wrap that in catching and forget about it? :D Or at least flip the condition to browser targets?

@yoodame
Copy link

yoodame commented Dec 29, 2024

Confirmed that the latest version does fail in RN. Produces this error from the taoensso.sente.js file.

Uncaught Error
goog.global.addEventListener is not a function
* A module failed to load due to an error and `AppRegistry.registerComponent` wasn't called.

@ptaoussanis ptaoussanis self-assigned this Dec 30, 2024
@ptaoussanis
Copy link
Member

Why not just wrap that in catching and forget about it?

No good reason, just lack of familiarity with React Native :-) If just catching would be preferable, I'll make that change now 👍

@ptaoussanis
Copy link
Member

@yoodame Thank you Yaw!

Do either of you maybe have a project you could recommend that's using both Sente and React Native? Would like to confirm that this is the only thing preventing RN support.

@yoodame
Copy link

yoodame commented Dec 30, 2024

I am currently working on an RN app with the latest Fulcro + Fulcro inspect tool. I've found 2 issues thus far. This one and another on encore - taoensso/encore#81

@awkay
Copy link
Author

awkay commented Dec 30, 2024

Verified that this fixes the error for RN as well. (tested both the new sente and encore version)

@awkay
Copy link
Author

awkay commented Dec 30, 2024

Thanks for the quick response!

@ptaoussanis
Copy link
Member

Great news, and thanks for the quick confirmation! I'll cut Sente v1.20 final later today and include these fixes 👍

(Will keep this issue open till the release is out)

@ptaoussanis
Copy link
Member

Preparing v1.20 release now, closing.

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

No branches or pull requests

3 participants