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

Feature/add destroy function to store #57

Conversation

rudionrails
Copy link
Contributor

@rudionrails rudionrails commented Jun 30, 2019

TL;DR
store.destroy() to remove event listeners, unsubscribe from store listeners or cleanup state.

Synopsis
This feature has 2 interfaces: either return a function from bundle.init() or explicitly declare bundle.destroy(). Both also work together. It is a 1-way final method - no option to revert or re-initialize the store again. See test/destroy.js for reference.

@rudionrails rudionrails changed the title Feature/add destroy funcion to store Feature/add destroy function to store Jun 30, 2019
@rudionrails rudionrails force-pushed the feature/add-destroy-funcion-to-store branch from fadc11c to 5ffd20b Compare July 21, 2019 16:28
@HenrikJoreteg
Copy link
Owner

This is really clean, I dig it!

I particularly like the pattern of returning the "unsubscribe" version of the function from adding event listeners and optionally returning a destroy method from init.

I think the only thing missing here is just some documentation for the /docs folder.

But overall, very nice.

@rudionrails
Copy link
Contributor Author

rudionrails commented Jul 29, 2019

Glad you like it so far. I would still need some help with this stuff here (may be affected by it)

Otherwise, I can add documentation on this for sure

@rudionrails rudionrails force-pushed the feature/add-destroy-funcion-to-store branch from 148fb0b to a750efe Compare July 29, 2019 20:05
@rudionrails
Copy link
Contributor Author

Updated docs to cover store.destroy, bundle.destroy and bundle.init. Would be happy for a merge now :)

@HenrikJoreteg
Copy link
Owner

I'm still on vacation in Sweden. Thanks for your patience. Will try to review properly and merge sometime after I get back.

@HenrikJoreteg HenrikJoreteg merged commit f715bd2 into HenrikJoreteg:master Sep 5, 2019
@HenrikJoreteg
Copy link
Owner

hey @rudionrails! I finally had a chance to do this.

I ended up actually removing the explicit bundle.destroy() support. Instead you have to return a cleanup method from init().

I did this because I didn't want to introduce multiple ways to do the same thing. And especially in the case of event listeners it seemed more likely that you've have the relevant listeners available in scope by doing it in init. Plus... it more closely matches patterns established elsewhere in the code.

Hope that's ok.

Anyway... released at 26.0.0 Thanks again!

@rudionrails
Copy link
Contributor Author

Perfect. Thanks :)

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