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

Preliminary Scopes Implementation #102

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aidanm3341
Copy link
Member

An initial attempt at implementing scopes to get some ideas and discussions going. I'm wondering if there is a better way to do the instantiation using Needle...

@Davidhanson90
Copy link

I think we need some additional docs for this change which details about how messages propagate up and down scopes.

@Davidhanson90
Copy link

What are we doing about scope disposal. I think it needs to be part of this process.

@aidanm3341
Copy link
Member Author

aidanm3341 commented Sep 9, 2024

What are we doing about scope disposal. I think it needs to be part of this process.

That's a good point, there should be a mechanism for disposing of a scope similar to what exists for channels. This also made me realise another point - given that messages on a channel trickle down to the same channel in child scopes, the existing disposal logic should dispose of the same channel in the children.

* @returns An instance of the messagebroker that matches the scopeName provided
*/
public createScope(scopeName: string): IMessageBroker<T> {
const scope = rootInjector.createScope(scopeName);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a needle scope? I understand why you're doing it - to allow injection of the messagebus instance for that scope. But how would I ever get access to the scoped injector. It's not bound to any UI element that will ui children to inject it.
I'm not saying this is incorrect - it's possibly something that @Davidhanson90 has specified but I don't understand how it would work.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right that it's probably not needed. I should have already removed it though, I think this was an outdated bit of code you commented on.

Copy link

@Davidhanson90 Davidhanson90 left a comment

Choose a reason for hiding this comment

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

Some minor suggestions. Where do we stand on destroy or scopes?

/**
* A reference to the parent scope if this is not the root node in the tree of scopes. If this is the root, it's undefined.
*/
readonly parent?: IMessageBroker<T>;

Choose a reason for hiding this comment

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

I think the parent may have different generic params. We are assuming all the tree is uniform. Its fine but something to considerr.

/**
* A list of all child scopes that have been created on this instance of the broker.
*/
readonly scopes: IMessageBroker<T>[];

Choose a reason for hiding this comment

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

Do we want to go parent -> children?

@@ -180,4 +206,12 @@ export class MessageBroker<T = any> implements IMessageBroker<T> {
): channel is RequiredPick<IChannelModel<T[K]>, 'config' | 'subscription'> {
return channel != null && channel.subscription != null;
}

public get parent(): IMessageBroker<T> | undefined {

Choose a reason for hiding this comment

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

you may want a isRoot() property to determine of the top of the tree

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