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
Add
connect()
,disconnect()
, deprecatesetAutoReconnect()
,resume()
, andconnected
#9439Add
connect()
,disconnect()
, deprecatesetAutoReconnect()
,resume()
, andconnected
#9439Changes from all commits
26fcaf4
1126595
8b2e788
df6192c
879400f
660a70f
b8edf82
a05fe75
11c7cd8
dd1a872
94f1401
5847ea0
1ad1809
a8c083b
c6bbc93
5a58350
b7a4697
05271fb
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.
Why not add new APIs on class, propagate and wait some time, and then add them here, but not make them optional?
Alternatively, we can make them non-optional from start - I do not see big problem here. As long as we are adding implementation in parallel, package bump would be noop in the future.
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.
Yeah, that makes sense. I need to create a follow up PR to add
connect()
anddisconnect()
toIFluidContainer
anyway, so we could just add in the functions to bothIContainer
andIFluidContainer
in one PR.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.
Should we add
connect()
anddisconnect()
as optional right away, especially since you're mentioning them here, and to help consumers transition? I don't think adding an optional thing is ever breaking (but could be wrong).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 actually had them added as optional earlier but changed it after discussion here. To summarize, I will have to add the functions to
IFluidContainer
in a separate PR anyway, so I am planning to add them toIContainer
andIFluidContainer
in one PR. This way we won't have to make another PR to change it from optional to mandatory functions.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.
Ok. Feel free to ignore what I'm about to say then :)
In general, I believe having a property absent is completely equivalent to having it present but optional, in terms of compatibility. It's just a hint saying "you may find something here...". So I would think you can and should add the optional functions to both interfaces in this PR.
To say the above another way, changing a member from optional to mandatory is equivalent to adding it when it's missing. At least I think so - correct me if I'm wrong, this stuff really bends my brain.
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.
@markfields The main problem with going from optional to mandatory I think is that it can still be a breaking change to anyone with an external implementation of the interface, where an implementation/object matching the interface suddenly no longer does so. It sounds like this shouldn't be an issue in this case though, but if there's a breaking change no matter how you slice it there's some appeal to doing just one step instead of two
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'm saying something different, I agree with that for sure.
Going from nothing to optional is never any trouble. Going from optional to required is trouble - it's as much trouble as adding a required thing outright. So might as well add optional earlier than later to give people an easier time onboarding/preparing.
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's the appeal of doing one step instead of two?
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 agree with @heliocliu and I'm not sure why this was removed from this PR? The goal of this PR is to introduce connect and disconnect at the Container level. Since we are enforcing using IContainer we are telling devs to use APIs as part of depreciation that aren't actually exposed.
Seems like the right step is to add them as optional as part of this PR and include the version in the Breaking.md that we will be making the breaking change. Then make them mandatory in next (or whatever breaking change we decide)
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.
Fewer steps 🙃
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.
@anthony-murphy @heliocliu, would this be considered a breaking change that needs to be merged into next instead of main? Initially, I thought it would be considered non-breaking to add
connectionState
toIFluidContainer
andFluidContainer
, but the new type validation showed that the previous version is not forward compatible. Thoughts on if this is non-breaking and can be included in this PR?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.
Forward incompatibility (probably) won't cause compile breaks, but you still need to consider if it will cause runtime compatibility breaks. It looks like that won't be the case here since you're not calling into the loader layer changes from other layers, but it does mean you won't be able to use the new apis in main. This is what Vlad was talking about in #9439 (comment) as well
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 think this comment was referring to the new
connect()
/disconnect()
functions being added toContainer
before adding them onIContainer
. In this case,connectionState
already exists onContainer
, so it should be able to work in main. Is that correct?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.
yeah that will be fine
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.
Please add documentation
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.
These should actually just inherit the docs from the interface.
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.
Please add documentation.
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.
@vladsud -- Do you know if this distinction of "ManualReconnect" was leveraged much in telemetry analysis? How big a loss is this? I don't quite get the semantics of it honestly, but wanted to double-check.
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.
Probably? It's useful to learn how quickly document reflects latest changes after user comes back to a tab that was inactive for very long time. That was the reason for this property on connection events. I think it's interesting data to have, but with these changes I think responsibility is pushed to consumers, and I'm fine with it. But we should look closer if consumers have everything, they need to be successful here.