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

feat(messaging): Create container in its own method to easier subclassing #8008

Merged
merged 1 commit into from
Jun 15, 2020

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Jun 15, 2020

What it does

Move code in its own method to make it easy when subclassing
It allows to grab container created without requiring to override the full method

How to test

Nothing should break

Review checklist

Reminder for reviewers

Change-Id: Idb2a1fde259eefd8c0485a8632233565f537b5a5
Signed-off-by: Florent Benoit fbenoit@redhat.com

…sing

Change-Id: Idb2a1fde259eefd8c0485a8632233565f537b5a5
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
protected getConnectionChannelHandlers(socket: ws): MessagingContribution.ConnectionHandlers<WebSocketChannel> {
const connectionContainer = this.container.createChild();
protected createSocketContainer(socket: ws): Container {
const connectionContainer: Container = this.container.createChild() as Container;
Copy link
Member

Choose a reason for hiding this comment

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

(nit) ts should be able infer types here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this is what I was thinking as well but without the as Container I've

src/node/messaging/messaging-contribution.ts(182,15): error TS2740: Type 'Container' is missing the following properties from type 'Container': _middleware, _bindingDictionary, _snapshots, _metadataReader, and 5 more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

ah ok, looks good to merge

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

LGTM

@akosyakov akosyakov added the messaging issues related to messaging label Jun 15, 2020
@benoitf benoitf merged commit 2f64828 into eclipse-theia:master Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
messaging issues related to messaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants