Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Clarify use of sync. Closes #2298 #2303
Clarify use of sync. Closes #2298 #2303
Changes from 10 commits
861d119
f992d55
2e707fe
26bb51f
21567d4
e34bf4c
f734aee
dd09a2f
d71ad21
8a3625e
d05123d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is all of this new logic and why has it been added here?
I'm not sure this is a good idea... did you test this with multiple groups? It's possible that
beforeDestroy
could be called as a result of the group switcher switching to a different group. If that happens it would break the joining attempt on the new group that we're trying to join.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really new logic, rather redefining the old logic that called
/sync
, which is unsafe to call.I don't see how that'd break anything. The
onDestroy
is there to callrelease
, which is the correct thing to do after callingretain
. If the view is destroyed, there's no point in keeping the group around (if it shouldn't be released, the reference count should indicate that). Note that before this only calledsync
and it also didn't affect the reference count.No, I'm not sure how that'd be done as joining a group would open a new URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd create and join group1 as u1. Then u2 (in a separate container tab or browser) would send you an invite link to group2, but before u1 visits it, u2 would log out. Then u1 visits the link to join u2's group2 and get stuck on this pending page. Then they'd switch back to their own group. Then u2 would log back in.
I see. Could you add a comment here as well then to explain where the "real" (persistent) retain happens on this group and why we're calling ephemeral retain here instead of
sync
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this and nothing broke, as expected, since this is merely a UI concern.
Done. Also added a comment to the effect that
vue
files should not be managing references.