-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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] set auto-subscription to undefined when update store to falsy value #7693
Conversation
d741000
to
8611af9
Compare
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.
LGTM! I posted 1 question.
export function subscribe_dynamic_store(store, callback) { | ||
if (store == null) { | ||
callback(undefined); | ||
return noop; | ||
} | ||
const unsub = store.subscribe(callback); | ||
return unsub.unsubscribe ? () => unsub.unsubscribe() : unsub; | ||
} |
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.
[ask]
This is a question but why did you create the new function?
Why adding callbacks.forEach(callback => callback(undefined));
to subscribe
is not good?
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.
sorry didn't understand the question.
You are saying using what statement instead of which function?
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.
Already we have subscribe function.
And the difference between subscribe_dynamic_store
and subscribe
is only line number 77. (callback(undefined);
)
So I thought that we can update subscribe
without creating the new function subscribe_dynamic_store
.
But I believe you have a reason why subscribe_dynamic_store
is necessary. So I would like to know about it.
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.
Hmm.. I can't remember why I did that.
I know we need to call callback(undefined)
so that when store
changed to undefined, we should update $store
to undefined too.
The subscribe
was used when it assumes that the store variable is static, so does not need to call callback
to set the $store
to undefined.
But let me look at it again and see if I can reuse the code.
Close in favor of #7947 |
Fixes #7555
Before submitting the PR, please make sure you do the following
[feat]
,[fix]
,[chore]
, or[docs]
.Tests
npm test
and lint the project withnpm run lint