Implement User/App listener #4522

takameyer · 2022-04-25T15:06:08Z

What, How & Why?

Add change listener for app and user

Add a member variable to store listener tokens to app and user.
Store the shared pointer to app and user as a member variable on User and App classes to ensure that the tokens are moved correctly and not copied.
Write tests for app listener.

This closes #4455

☑️ ToDos

📝 Changelog entry
📝 Compatibility label is updated or copied from previous entry
🚦 Tests
📱 Check the React Native/other sample apps work if necessary
📝 Public documentation PR created or is not necessary
💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

typescript definitions file is updated
jsdoc files updated
Chrome debug API is updated if API is available on React Native

CHANGELOG.md

docs/sync.js

src/js_app.hpp

src/js_sync.hpp

src/js_user.hpp

tests/js/app-tests.js

tests/spec/unit_tests.js

tomduncalf

Not completed review yet, just submitting a few comments

docs/sync.js

tomduncalf · 2022-04-29T08:44:50Z

src/js_app.hpp

+    using CallbackTokenPair = std::pair<Protected<typename T::Function>, AppToken>;
+    std::forward_list<CallbackTokenPair> m_notification_tokens;
+
+    SharedApp m_app;


Did you conclude that trying to extend shared_ptr directly was leading to issues?

I was having issues that went away by implementing it with a member. That being said, it's possible that it could work extending the shared_ptr directly. Maybe a second look would be worthwhile, but this time using ->get() to retrieve a reference to app instead of dereferencing directly.

That didn't work the way I had hoped. Found some reading on inheriting shared_ptr though..
https://stackoverflow.com/a/30309840/192046

src/js_app.hpp

kraenhansen · 2022-04-29T09:44:16Z

I just had a quick look so far. It looks like a couple of files were committed by accident:

tests/.lock
tests/realm-bundle.realm.backup-log

Now that we're adding to the Realm JS types, do you plan to extend Realm Web with this as well? Possibly in a follow up PR? Perhaps we need to revisit the scope to clarify that?

A heads up: Porting this to v11 will most likely involve the use of the NotificationBucket API that we introduced for all other things that are observable.

takameyer · 2022-04-29T09:49:42Z

Now that we're adding to the Realm JS types, do you plan to extend Realm Web with this as well? Possibly in a follow up PR? Perhaps we need to revisit the scope to clarify that?

@kraenhansen I do not have realm-web on my radar, this was completely focused on @realm/react. We should plan time for that. I would need to familiarize myself with the project and its test framework. I think we should scope updates to realm-web in a separate agenda.

takameyer · 2022-04-29T09:51:30Z

A heads up: Porting this to v11 will most likely involve the use of the NotificationBucket API that we introduced for all other things that are observable.

This should be applied to everything then (objects, collections, realm), and should be planned.

kraenhansen · 2022-04-29T09:57:44Z

This should be applied to everything then (objects, collections, realm), and should be planned.

It already has been implemented for the other observables.

takameyer · 2022-04-29T10:01:20Z

It already has been implemented for the other observables.

Then I will have to make a second PR for the v11 branch.

src/js_app.hpp

tomduncalf

Nice work, just a few small comments

src/js_app.hpp

src/js_user.hpp

src/js_app.hpp

src/js_user.hpp

Add a member variable to store listener tokens to app and user. Store the shared pointer to app and user as a member variable on User and App classes to ensure that the tokens are moved correctly and not copied. Write tests for app listener.

Co-authored-by: Tom Duncalf <[email protected]>

Co-authored-by: Kenneth Geisshirt <[email protected]> Co-authored-by: Tom Duncalf <[email protected]>

* Refactored NotificationBucket to take a Token type * Using NotificationBucket for App and User

Co-authored-by: FFranck <[email protected]>

cla-bot bot added the cla: yes label Apr 25, 2022

takameyer force-pushed the andrew/rr-user-listener branch 5 times, most recently from f0815d5 to 5f66dce Compare April 29, 2022 07:52

takameyer marked this pull request as ready for review April 29, 2022 07:55

takameyer requested review from kneth, tomduncalf, kraenhansen and fronck April 29, 2022 08:05

kneth approved these changes Apr 29, 2022

View reviewed changes

tomduncalf suggested changes Apr 29, 2022

View reviewed changes

takameyer force-pushed the andrew/rr-user-listener branch from 6462afa to ea7dc6f Compare April 29, 2022 09:52

tomduncalf reviewed Apr 29, 2022

View reviewed changes

src/js_app.hpp Outdated Show resolved Hide resolved

tomduncalf approved these changes Apr 29, 2022

View reviewed changes

kraenhansen approved these changes May 2, 2022

View reviewed changes

takameyer force-pushed the andrew/rr-user-listener branch from 77c40d7 to 89beeb2 Compare May 3, 2022 05:31

kraenhansen mentioned this pull request May 3, 2022

Backport of NotificationBucket #4550

Merged

6 tasks

fronck suggested changes May 3, 2022

View reviewed changes

takameyer added 5 commits May 3, 2022 13:26

Add change listener for app and user

1e60a0d

Add a member variable to store listener tokens to app and user. Store the shared pointer to app and user as a member variable on User and App classes to ensure that the tokens are moved correctly and not copied. Write tests for app listener.

Update changelog

bd5e5e1

Add typescript types

a0b8b2a

Add jsdocs

2e4cf0c

PR updates

c06735b

kraenhansen and others added 7 commits May 3, 2022 13:27

Adding stubs for App and User listeners

cafe642

Apply suggestions from code review

3354eb8

Co-authored-by: Tom Duncalf <[email protected]>

Apply suggestions from code review

f77f4a5

Co-authored-by: Kenneth Geisshirt <[email protected]> Co-authored-by: Tom Duncalf <[email protected]>

Update description of listener events

14d2510

Add Remove All Listeners

88436e9

PR fixes

c8f018b

Add removeAllListeners to realm-web

86c6320

kraenhansen force-pushed the andrew/rr-user-listener branch from 89beeb2 to 86c6320 Compare May 3, 2022 11:28

kraenhansen and others added 4 commits May 4, 2022 07:59

Use NotificationBucket for App & User (#4551)

95748c5

* Refactored NotificationBucket to take a Token type * Using NotificationBucket for App and User

Apply suggestions from code review

be256ed

Co-authored-by: FFranck <[email protected]>

Apply suggestions from code review

61d1b40

Co-authored-by: FFranck <[email protected]>

Add header comment blocks to new functions

448de72

takameyer force-pushed the andrew/rr-user-listener branch from 618b2e8 to 448de72 Compare May 4, 2022 10:47

Small refactorings

5365ed3

takameyer merged commit b4d6033 into master May 4, 2022

takameyer deleted the andrew/rr-user-listener branch May 4, 2022 13:54

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement User/App listener #4522

Implement User/App listener #4522

takameyer commented Apr 25, 2022 •

edited

Loading

tomduncalf left a comment

tomduncalf Apr 29, 2022

takameyer Apr 29, 2022

takameyer Apr 29, 2022

kraenhansen commented Apr 29, 2022

takameyer commented Apr 29, 2022

takameyer commented Apr 29, 2022

kraenhansen commented Apr 29, 2022

takameyer commented Apr 29, 2022

tomduncalf left a comment

Implement User/App listener #4522

Implement User/App listener #4522

Conversation

takameyer commented Apr 25, 2022 • edited Loading

What, How & Why?

☑️ ToDos

tomduncalf left a comment

Choose a reason for hiding this comment

tomduncalf Apr 29, 2022

Choose a reason for hiding this comment

takameyer Apr 29, 2022

Choose a reason for hiding this comment

takameyer Apr 29, 2022

Choose a reason for hiding this comment

kraenhansen commented Apr 29, 2022

takameyer commented Apr 29, 2022

takameyer commented Apr 29, 2022

kraenhansen commented Apr 29, 2022

takameyer commented Apr 29, 2022

tomduncalf left a comment

Choose a reason for hiding this comment

takameyer commented Apr 25, 2022 •

edited

Loading