Skip to content

Channels visibility improvements#558

Merged
QuintinWillison merged 4 commits intomasterfrom
feature/encapsulate-channels
Feb 14, 2020
Merged

Channels visibility improvements#558
QuintinWillison merged 4 commits intomasterfrom
feature/encapsulate-channels

Conversation

@QuintinWillison
Copy link
Copy Markdown
Contributor

The quick win was making the channels field on realtime instances final - preventing that reference from being trampled.

The larger change was a bit more complex, involving (both) the Channels interfaces we have. They were classes but are now interfaces with private implementations, solving the following accessibility issues:

  1. Constructor: It should never have been accessible, however it was also causing an issue for Kotlin developers because the lack of a new keyword makes it less obvious that you're creating an instance of an inner class.
  2. Underlying map implementation: Allowing anybody in possession of a Channels reference to mutate the channels map without the Ably implementation knowing.
  3. Internal methods: Called by the connection manager but could equally be called by anybody in possession of a Channels reference without the connection manager knowing.

Aside

I wanted to push further and deprecate the channels fields on both AblyRealtime and AblyBase, however because AblyRealtime is a subclass of AblyBase it was not as easy as just creating a getChannels() method. The problem is that fields can shadow superclass fields and change their type at the same time, even to an incompatible type, but methods can't. And the Channels returned by base are of a completely different class to the Channels returned by realtime (due to the fact that we also have two forks of ChannelBase upon which each is ultimately reliant!).

…ime and REST.

Doing as much as I can without this being a breaking change.
As far as we are aware, this is only likely to be a breaking change for those mocking AblyRealtime.channels for testing purposes.
@QuintinWillison QuintinWillison added the bug Something isn't working. It's clear that this does need to be fixed. label Feb 13, 2020
@QuintinWillison
Copy link
Copy Markdown
Contributor Author

Yes, kids, it's real. Always, always, always run tests before you push!

@QuintinWillison QuintinWillison removed the request for review from paddybyers February 13, 2020 13:19
@QuintinWillison
Copy link
Copy Markdown
Contributor Author

Sorry, @paddybyers, I've broken all kinds of tests. Shall fix those and then re-add my request for review once done.

Copy link
Copy Markdown
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@QuintinWillison QuintinWillison merged commit eacc61d into master Feb 14, 2020
@QuintinWillison QuintinWillison deleted the feature/encapsulate-channels branch February 14, 2020 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working. It's clear that this does need to be fixed.

Development

Successfully merging this pull request may close these issues.

2 participants