-
-
Notifications
You must be signed in to change notification settings - Fork 828
Refactor integration manager handling into a common place #3301
Conversation
It was already in a common place, but this is the boilerplate for supporting multiple integration managers, and multiple integration manager sources. For element-hq/element-web#4913 / element-hq/element-web#10161
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise lgtm
import {IntegrationManagerInstance} from "./IntegrationManagerInstance"; | ||
|
||
export class IntegrationManagers { | ||
static _instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has the same problem as module-level variables: you'll get a different instance if you ever access this from a file in riot-web.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I should test it :|
can we just rewrite it all in a language that doesn't do dumb things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the riot-web layer explodes when it sees the flow imports...
} | ||
|
||
openNoManagerDialog(): void { | ||
// TODO: Is it Integrations (plural) or Integration (singular). Singular is easier spoken. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we typically say "integration manager", and grammatically I think it makes sense that a "thing
manager" would manage multiple thing
s, so let's try to standardise around "integration manager".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unbelievably appreciative of this comment. I'm going to go delete a bunch of extraneous s's now.
It was already in a common place, but this is the boilerplate for supporting multiple integration managers, and multiple integration manager sources.
For element-hq/element-web#4913 / element-hq/element-web#10161