-
Notifications
You must be signed in to change notification settings - Fork 33
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: added checks on creating events and added method for unregister… #144
base: main
Are you sure you want to change the base?
Conversation
@Mohmn did you test this locally? |
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.
Maybe instead of a function to remove methods, we can return a unsubscribe
method when subscribing on
Yep i have tested it locally |
I donot see how it will be helpful, can u state the benefits of it |
@RubenSmn you mean like the way of useEffect returning function? if that's the goal, I think the event's |
I mean these days its a very common pattern to get back a |
Should I do both things |
I feel like its weird to have to call an "extra" method and pass the I think that for the developer experience it is more logical to just receive an |
But maybe there are some cases we need to do the unscribe separately? |
Our subscriptions are based on the Seperate Method (off)// Parent
const Parent = () => {
const myHandler = () => {
// do stuff
};
map.on("this-event", myHandler);
return (
<Child handler={myHandler} />
);
};
// Child
const Child = ({ handler }) => {
// needs the exact instance of the "handler" function.
// needs an extra "map" instance
// needs the "this-event" name
const onClick = () => {
map.off("this-event", handler);
};
return (
<button onClick={onClick}>Remove the handler from the map</button>
);
}; Unsubscribe Method (callback)// Parent
const Parent = () => {
const myHandler = () => {
// do stuff
};
const unSubscribe = map.on("this-event", myHandler);
return (
<Child unSubscribe={unSubscribe} />
);
};
// Child
const Child = ({ unSubscribe }) => {
// only cares about unsubscribing
const onClick = () => {
unSubscribe();
};
return (
<button onClick={onClick}>Remove the handler from the map</button>
);
}; |
@RubenSmn I'm confused, what's your preference? callback? |
@dadiorchen i think that @RubenSmn here is trying to imply that whether we use handleoff(implicit unsubscribe method) or callback (subscribe returning unsubscribe), we need to store the reference for it. |
Ruben's point is now making sense to me , so which way should we go now |
@Mohmn let me take some time to understand this |
@dadiorchen yes my preference is the callback. I think it will reduce the possibility for errors and provides a better developer experience! |
…ing events"
fixes #134