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

Trigger onReady callback on resubscribe #8754

Closed
wants to merge 6 commits into from

Conversation

@zimme
Copy link
Contributor

@zimme zimme commented Jun 2, 2017

This fixes #1173.

Tests needs updating. I know what needs updating,
I just need to update commends and stuff before
committing.

This fixes #1173.
@abernix abernix changed the title Trigger ready callback on resubscribe [Work in progess] Trigger ready callback on resubscribe Jun 2, 2017
zimme added 2 commits Jun 3, 2017
@zimme
Copy link
Contributor Author

@zimme zimme commented Jun 3, 2017

This should be ready now.

@zimme zimme changed the title [Work in progess] Trigger ready callback on resubscribe Trigger ready callback on resubscribe Jun 4, 2017
Copy link
Member

@hwillson hwillson left a comment

Thanks @zimme - this all looks great! I'm re-running the tests now, but assuming they pass we should be all set to get this merged.

@hwillson
Copy link
Member

@hwillson hwillson commented Jun 7, 2017

One question - do you think we need to worry about instances where people are counting on the current (buggy) behaviour? For example, #1173 (comment). If so this might be a breaking change.

@zimme
Copy link
Contributor Author

@zimme zimme commented Jun 7, 2017

I guess we might need a short discussion about if we want to keep the current behaviour and document that you can use handle.ready() if you want the behaviour that this PR brings instead.

However I do think this should be classified as a bug and fixed because there are more people that assume that this function would work the way it would with this PR applied.

So maybe a warning in History.md about a potential breaking change for people relying on this broken behaviour.

On another note, do we have any way of achieving what the developers in that comment were doing if we apply this patch?

@abernix abernix added this to the Release 1.5.1 milestone Jun 7, 2017
@mitar
Copy link
Contributor

@mitar mitar commented Jun 7, 2017

I would vote for merging this in and adding a history entry.

@hwillson
Copy link
Member

@hwillson hwillson commented Jun 8, 2017

Thanks @mitar - that was the consensus on our triage call today as well (we were just waiting for the failed tests to re-run). This will be merged shortly (@abernix is planning on adding a small adjustment to the History.md to explain how this could break backwards compatibility in some cases).

To reflect that #8754 is a breaking change, we should bump the major version here.
@sebakerckhof
Copy link
Contributor

@sebakerckhof sebakerckhof commented Jun 14, 2017

Question: Why, in the case the existing subscription is not yet ready, is the ready callback overwritten instead of added? I mean the code has this comment:

 the semantics we provide is
// that at the time the sub first becomes ready, we call the last
// onReady callback provided, if any.`

But this doesn't really make sense if you think about a component-based GUI, since the onReady callback will often be used to for example hide a loading indicator.

So that means if you have 2 different components subscribing to the same thing more or less at the same time, and both use the 'onReady' callback to hide a loading indicator inside the component, only one will be called and thus one component will keep showing the loading indicator?

@zimme
Copy link
Contributor Author

@zimme zimme commented Jun 14, 2017

@sebakerckhof, even if 2 components were to subscribe to the same publication there would still be 2 separate subscription instances with their own onReady callbacks? So I don't think they would contaminate each other.

I believe this might be more related to conditionally calling subscribe for the same publication in an autorun with different callbacks based on the condition?

@abernix abernix changed the title Trigger ready callback on resubscribe Trigger onReady callback on resubscribe Jun 14, 2017
[no ci]
@sebakerckhof
Copy link
Contributor

@sebakerckhof sebakerckhof commented Jun 15, 2017

@zimme Yeah, nevermind, I missed the inactive part. I thought this code was to avoid sending the same subscription twice to the server, but it's about autorun behavior.

benjamn added a commit that referenced this pull request Jun 21, 2017
To reflect that #8754 is a breaking change, we should bump the major version here.
benjamn added a commit that referenced this pull request Jun 21, 2017
[no ci]
@benjamn
Copy link
Member

@benjamn benjamn commented Jun 21, 2017

Merged into Release 1.5.1 as part of 1.5.1-beta.3.

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

Successfully merging this pull request may close these issues.

6 participants