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

Refactor/rename messaging contribution push param #10996

Conversation

harbin1053020115
Copy link

What it does

Rename message-contirbution.ts(packages/core/src/node/messaging/messaging-contribution.ts) ConnectionHandlers.push param, which is changing channel to socket.

How to test

No need to test.

Review checklist

Reminder for reviewers

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 👍

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

I'm not sure I fully understand the reasons for the change, it is only updating the variable name in the callback, uses of MessagingContribution.push still use channel (verified with find all references).

@harbin1053020115
Copy link
Author

harbin1053020115 commented Apr 6, 2022

Thank you for your contribution 👍

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

I'm not sure I fully understand the reasons for the change, it is only updating the variable name in the callback, uses of MessagingContribution.push still use channel (verified with find all references).

The original channel param name is misunderstanding with the channel concept in MessagingContribution.handleChannels method, but actually it means socket.

@vince-fugnitto
Copy link
Member

The original channel param name is misunderstanding with the channel concept in MessagingContribution.handleChannels method, but actually it means socket.

I believe socket might also be confusing since it accepts any generic connection (likely the best name).
Currently we have WebSocketChannel and Socket that make use of ConnectionHandlers:

protected readonly wsHandlers = new MessagingContribution.ConnectionHandlers<Socket>();

protected readonly channelHandlers = new MessagingContribution.ConnectionHandlers<WebSocketChannel>();

protected readonly channelHandlers = new MessagingContribution.ConnectionHandlers<WebSocketChannel>();

@harbin1053020115
Copy link
Author

harbin1053020115 commented Apr 6, 2022

so, if this is meaningful:

push(spec: string, callback: (params: MessagingService.PathParams, connection: T) => void): void {
    const route = new Route(spec);
    // here is the change, or you can look at the lastest changes.
    this.handlers.push((path, channel: T) => {
        const params = route.match(path);
        if (!params) {
            return false;
        }
        callback(params, channel);
        return route.reverse(params);
    });
}

@@ -211,7 +211,7 @@ export namespace MessagingContribution {

push(spec: string, callback: (params: MessagingService.PathParams, connection: T) => void): void {
const route = new Route(spec);
this.handlers.push((path, channel) => {
this.handlers.push((path, channel: T) => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the value in this change?

We can already infer the type from handlers:

protected readonly handlers: ((path: string, connection: T) => string | false)[] = [];

and the editor gives us hover information that channel is of type T.


What exactly is the change attempting to achieve?

Copy link
Author

Choose a reason for hiding this comment

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

I see, please ignore this pr

Copy link
Member

Choose a reason for hiding this comment

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

No problem! If there are improvements or features you'd like to work on please don't hesitate to contribute and we'd be happy to review them :)

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.

3 participants