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

fix(core):cannot remove effects correctly when use addEffect/addAfter… #2357

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

dev-meta
Copy link
Contributor

@dev-meta dev-meta commented Jul 5, 2022

when use addEffect/addAfterEffect/addTail, cannot remove effects(subscriptions) correctly.

I use Set instead of Array to fix this problem.

thanks for Creating a such perfect project.

sincerely

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 5, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1d671c0:

Sandbox Source
example Configuration

subs.push(callback)
return () => void subs.splice(index, 1)
function createSubs(callback: GlobalRenderCallback, subs: Set<SubItem>): () => void {
const sub = {callback}
Copy link
Member

Choose a reason for hiding this comment

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

looks good i think we should use set, but just that, why do you wrap it into an object here?

Copy link
Contributor Author

@dev-meta dev-meta Jul 6, 2022

Choose a reason for hiding this comment

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

If don't use object to wrap the callback, pass in the identical callback function more than once and will only one is left.
for example just like the below.

const log = (ts) = console.log('log', ts);
addEffect(log);
addEffect(log);
addEffect(log);

If so, it will confuse the developers.

sincerely.

@dev-meta dev-meta requested a review from drcmda July 8, 2022 01:19
@drcmda drcmda merged commit 8ad0559 into pmndrs:master Jul 8, 2022
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.

2 participants