Skip to content

Add export for dataSubscriptionRequest #1

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikeperri
Copy link

Explained in the README. Also fixes a bug where EMPTY_FUNC actually returns undefined instead of empty object.


request = (endpoint, updatedCallback) => {
const subscription = this.createSubscription(endpoint);
subscription.run();
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should run after the below listener is attached.

@@ -49,6 +49,15 @@ class Store {
dump = () => {
return cloneDeep(this._store);
}

request = (endpoint, updatedCallback) => {
Copy link
Owner

Choose a reason for hiding this comment

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

In order to be consistent with regular subscriptions and to provide more flexibility it should take and pass paramsFunc to the subscription. Or it should take params and pass () => params if it's a one time call.

@madshall
Copy link
Owner

Thank you @mikeperri, the feature is useful, but I see one issue here - memory leak. The created subscription never gets cleaned up by GC because it has an event listener attached to it. Every time request is called it will leave the entire subscription object (+ event listener) in memory after itself. Also, the underlying entity will never get destructed either, unless a react component with the same subscription exists. In that case it will take care of GC for the entity. Proper GC is very important for this package. Let me think about how we can do it here and I'll get back to you.

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