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/connection events #147

Merged
merged 10 commits into from
Feb 14, 2024
Merged

Fix/connection events #147

merged 10 commits into from
Feb 14, 2024

Conversation

jankapunkt
Copy link
Member

@jankapunkt jankapunkt commented Feb 12, 2024

Summary

In rare occasions I found that when using NetInfo: null in order to manually manage connect/disconnect, I can't get the App connected, due to the events being fired before the listeners were attached internally.

I therefore moved the code to call Data.dpp.connect at a point after the listeners were set up.
Should not break current builds.

Linked issue(s)

Involved parts of the project

Added tests?

Targeted Meteor release version

Reproduction

install @meteorrn/[email protected] to test

@bratelefant
Copy link
Collaborator

Cool, will test this in my staging env; from time to time I also have a similar issue (might be related) where especially when the app is suspended in iOS and then brought back up, the current Meteor.user() is undefined.

@jankapunkt
Copy link
Member Author

Can you please open an issue on the Meteor.user problem including a minimal repro code? I have some null user errors on my server and it might be related

@bratelefant
Copy link
Collaborator

Can you please open an issue on the Meteor.user problem including a minimal repro code? I have some null user errors on my server and it might be related

Will try to, however this seems really hard to reproduce. Sometimes got User._loginWithToken::: token is null in the logs; There's no check here in User.js, if the parameter value is not null. Anyway, this is off-topic with respect to this PR, so I'll file an issue and we can go on there

Copy link
Collaborator

@bratelefant bratelefant left a comment

Choose a reason for hiding this comment

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

Looks fine, works fine. For testing this (I'm using the "local collections" companion package) I needed to bump peerDependencies for that package; can you the version numbers of core to the peerDeps of the companion packages as well?

bratelefant
bratelefant previously approved these changes Feb 13, 2024
@jankapunkt jankapunkt mentioned this pull request Feb 13, 2024
do not save token Id, if value is null
@jankapunkt
Copy link
Member Author

Sorry reviews got dismissed after merge. I disabled that option for now. Can you please approve again?

Copy link
Collaborator

@bratelefant bratelefant left a comment

Choose a reason for hiding this comment

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

Look good, think we could merge in #150 as well

@jankapunkt jankapunkt merged commit e0fad19 into master Feb 14, 2024
9 checks passed
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.

2 participants