[plugin] support multiple windows per a backend #4521
Conversation
akosyakov
changed the title
|
@ensorrow Could you try? |
akosyakov
force-pushed
the
GH-4509
branch
2 times, most recently
from
e27fdca to
7112b44
Compare
|
need more time for testing in all che configuration |
Works very nice! |
How long? The current state is seriously broken. So would be good to have this fixed ASAP. |
|
@svenefftinge it is like this since the beginning and next release of theia is for the end of this month. |
|
@benoitf There are our products which depend on next and can use it now. We are waiting for it. Also PRs should not be stopped by testing all possible configurations of all using products. There should be self contained clients to test. |
|
@akosyakov you're introducing breaking changes on something you didn't initiated. So please wait the reviewers. It's being one day. We waited from your on some PR for several weeks. |
|
The point is that PRs can be reviewed by verifying master state against PR state for example apps, not end products. Which was done for this PR already twice. End products can migrate in 1 month as they pleased and report issues later.
It's not true, they were reviewed and did not work as before against example apps. |
funny enough that no one noticed it so far, as it's a seriously bad bug and should be fixed ASAP.
breaking changes should go onto master as early as possible, so downstream adopters can adjust. So please have a look, this PR is only five files long. |
I am sorry but it is not fair to tell end products to migrate and report in a month while trying to rush a PR review for an end product. I already know that Florent has planned to test this after his current review of another PR is done and then he will provide his feedback, probably tomorrow. I do not think we should ask for rushing reviews for our products. There are already many ways to manage one of fixes to a downstream product, it is just unnecessary to expect contributors to work on your schedule. |
|
Tomorrow is great. |
I did not mean it. I said that end products should NOT be used for testing, but there should be internal clients (example apps or tests). It's a case for this PR: it was already tested by 2 persons against example apps. End products can migrate when they want and don't need to test anything immediately. Otherwise, it's like testing Eclipse RCP against some RCP product on each change. Does not make any sense and make turnaround super slow. |
packages/plugin-ext/src/hosted/node/hosted-plugin-deployer-handler.ts
Outdated
Show resolved
Hide resolved
| export function bindHostedBackend(bind: interfaces.Bind): void { | ||
| bindCommonHostedBackend(bind); | ||
| bind(ConnectionContainerModule).toConstantValue(hostedBackendConnectionModule); |
There was a problem hiding this comment.
hello, simple question, is that ConnectionContainerModule is handling the destroy of the instances associated to the scope ? (closing the browser tab is destroying the instances of this scope ) ?
There was a problem hiding this comment.
yes, it should, instances only exist within connection scope and can access each other (be injected)
There was a problem hiding this comment.
closing the browser tab is destroying the instances of this scope
@svenefftinge it was my assumption. It could be good to implement, otherwise it could be hard to clean up without explicitly binding backend service.
There was a problem hiding this comment.
yes I assume we may have leaks on many items if instances are not destroyed/not notified ?
| server.setClient(client); | ||
| // FIXME: handle multiple remote connections | ||
| /* | ||
| client.onDidCloseConnection(() => server.dispose());*/ |
There was a problem hiding this comment.
I guess that server.dispose() would be useful in the new case (as we need to inform somehow that client is not connected if instances are not automatically destroyed ?
There was a problem hiding this comment.
It should be auto called when connection is closed, i wonder why it does not, will check
There was a problem hiding this comment.
no, i am wrong, it is not auto disposed, this line is needed, checking again and pushing
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
|
@benoitf should be good now, could you test again please |
|
@akosyakov tested, fixed the issue but I will have to add a follow-up PR to propagate the clientClosed() event to pluginRunners else they're not notified that client is disconnected now |
fix #4509
In order to verify:
It's a breaking change: some plugin bindings are scoped per a connection now. Clients, who contribute/rebind these bindings, will need to scope them per a connection as well.