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

Compression plugin #204

Merged
merged 6 commits into from Jun 20, 2017
Merged

Compression plugin #204

merged 6 commits into from Jun 20, 2017

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Mar 2, 2017

Closes #183

  • Adds a compression plugin, which uses get and set directly
  • Users can opt out of compression using an extra argument, ex) store.get('key', true), store.set('key', 'value', true)
  • Adds lz-string to plugins/lib
  • Simple compress and decompress tests

This is pretty simple. I'm not sure how I should be handling namespacing, or if I even need to.

@marcuswestin
Copy link
Owner

Super cool!! First v2 community plugin :)

Yes, the whole super_fn stuff is not super intuitive and a bit opaque. I'm looking for a better alternative if you can think of one.

super_fn has a property which is an array of the arguments that will be passed on. You can modify that array. I think I called it "args" but would have to check.

Maybe if super_fn receives arguments it should pass on those instead. The potential issue is if there are other plugins in between which accept more arguments than the original implementation, such as expire - I went with the current approach to ensure that they would always be passed on properly.

Let me know how it goes and if you have thoughts on this!

@w33ble
Copy link
Contributor Author

w33ble commented Mar 2, 2017

Yeah, I saw the args thing when I was debugging it. Originally, I assumed that super_fn just took the arguments you want, so i was calling super_fn(key, compressed) in set, and then when I saw it running, it was set to the uncompressed value. I was going nuts trying to figure out what I did wrong, until I dug a little deeper ;)

Let me tinker, I'm thinking I can just make it override the arguments passed into super_fn and fall back to whatever was originally passed in.

@w33ble
Copy link
Contributor Author

w33ble commented Mar 2, 2017

@marcuswestin rebased on master, and updated to use the get/set methods. I added an extra raw argument, so that even if the compression plugin is enabled, users can manually opt out when getting and setting data. It was also handy for the tests.

@marcuswestin
Copy link
Owner

Nice!

At this point multiple plugins with custom arguments don't work together :/ I think we may need to rethink optional args and plugins with custom args... Shouldn't block this PR though.

Will take a look tomorrow - it's 7p and I need to feed the baby :)

@w33ble
Copy link
Contributor Author

w33ble commented Mar 3, 2017

I can remove that raw argument if you'd like. I started writing it as additional methods, and I could go back to doing it that way.

@w33ble
Copy link
Contributor Author

w33ble commented Mar 31, 2017

Was this waiting on me for anything?

@w33ble w33ble closed this Apr 10, 2017
@marcuswestin
Copy link
Owner

No, it's waiting for me. Sorry @w33ble. Life happened :) I'll be taking this on in coming days.

@marcuswestin marcuswestin reopened this Jun 17, 2017
@marcuswestin marcuswestin self-assigned this Jun 17, 2017
@w33ble
Copy link
Contributor Author

w33ble commented Jun 18, 2017

That's fine. Hope all is ok!

I'll be honest, I've kind of lost interest in this PR. Figured it was better to close it than leaving it linger and worry about future updates. You're welcome to take it on, or use your own approach. I won't be offended if you don't use my commits ;).

marcuswestin added a commit that referenced this pull request Jun 20, 2017
@marcuswestin marcuswestin merged commit a14028d into marcuswestin:master Jun 20, 2017
@marcuswestin
Copy link
Owner

This has been released in v2.0.11. I only had to make some small changes to bring up to date and make tests pass for legacy IE browsers.

Thank you again @w33ble!

@mstralka
Copy link

This is cool but it should be noted that it causes errors if users already had data stored from an older version of storejs, and you add the compress plugin.

My app was using store/dist/store.everything.min.js, which includes all of the plugins automatically. When I updated from 2.0.4 to 2.0.11 and loaded the app in a browser that already had data in storage, it immediately threw this stack trace:

app.js:sourcemap:83912 Uncaught TypeError: t.charCodeAt is not a function
    at https://localhost/js/app.js:83912:10572
    at Object._decompress (https://localhost/js/app.js:83912:10671)
    at Object.decompress (https://localhost/js/app.js:83912:10524)
    at Object.t (https://localhost/js/app.js:83912:1373)
    at Object.(anonymous function) (https://localhost/js/app.js:83912:14911)
    at e (https://localhost/js/app.js:83912:14847)
    at Object.e (https://localhost/js/app.js:83912:1658)
    at Object.(anonymous function) (https://localhost/js/app.js:83912:14911)
    at e (https://localhost/js/app.js:83912:14847)
    at Object.e (https://localhost/js/app.js:83912:3139)

Loading the app in Chrome Incognito did not have this error.

My workaround was to switch to store/dist/store.modern.min.js

@marcuswestin
Copy link
Owner

marcuswestin commented Jul 11, 2017 via email

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.

None yet

3 participants