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

changed storage method names, added clear flag #86

Merged
merged 5 commits into from
Dec 13, 2020

Conversation

leastbad
Copy link
Contributor

set_storage and remove_storage follow the naming convention used by every other operation.

I added a new optional clear flag to the remove_storage method that optionally clears all keys.

@marcoroth
Copy link
Member

Wouldn't it make more sense to provide an extra clear_storage method that just clears the storage since it's also a method on it's own in the browser API?

@leastbad
Copy link
Contributor Author

Maybe! I would defer to Nate's preference here - clearing is definitely a kind of removal.

Fwiw I'm definitely influenced by the reality that this seems like it'd be usually very rarely.

@hopsoft
Copy link
Contributor

hopsoft commented Dec 13, 2020

I like the semantic change here for consistency, but don't think we should drop the item suffix. I also like the idea of adding clearStorage. One other nitpick that I'd like to address while we're at it is to move

const storage = type === 'session' ? sessionStorage : localStorage

directly under the line where we destructure vars out of the config.

Update: leastbad#1

@leastbad
Copy link
Contributor Author

Sounds good to me! I updated channels.rb with the new method names.

@hopsoft hopsoft merged commit d64b149 into stimulusreflex:master Dec 13, 2020
@leastbad leastbad deleted the storage branch December 13, 2020 17:53
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.

3 participants