marcuswestin / store.js

Cross-browser storage for all use cases, used across the web.
MIT License
14.02k stars 1.33k forks source link

Compression plugin #204

Closed w33ble closed 7 years ago

w33ble commented 7 years ago

Closes #183

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

marcuswestin commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Was this waiting on me for anything?

marcuswestin commented 7 years ago

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

w33ble commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Good point Mark - the compression read should probably have a fall back. Will take a look. On Tue, Jul 11, 2017 at 12:26 PM Mark Stralka notifications@github.com wrote:

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

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/marcuswestin/store.js/pull/204#issuecomment-314498731, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIDf_9e2ARpYnAg5wGfHTkQh0EbGkQVks5sM6JEgaJpZM4MQgYR .