Skip to content

Conversation

enver-bisevac
Copy link
Contributor

No description provided.

Copy link
Contributor

@knagurski knagurski left a comment

Choose a reason for hiding this comment

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

Great work. Just some observations/questions

Comment on lines +28 to +34
eventBus.on(Event.CONNECTED, () => {
this.stop();
});

eventBus.on(Event.DISCONNECTED, () => {
this.resume();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I think I get it now. So when the stream connects the polling stops. It might be a bit more obvious how it works if you pull this up to the parent class and let the PollingProcessor be a little bit more dumb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will make stop() and resume() public methods and from ClientFC make proper calls

import { StreamProcessor } from './streaming';
import * as events from 'events';

export class CfClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I'd consider this in the scope of this ticket, but since we no longer use "Continuous Features", should this be FFClient?

Copy link
Contributor

@knagurski knagurski left a comment

Choose a reason for hiding this comment

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

Worth considering the name of the CfClient class, but that can wait. Looks good

@enver-bisevac enver-bisevac merged commit 7f11125 into main Sep 7, 2021
@knagurski knagurski deleted the polling_processor branch July 13, 2023 13:07
@knagurski knagurski restored the polling_processor branch July 13, 2023 13:07
@knagurski knagurski deleted the polling_processor branch July 13, 2023 13:07
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