search

lmaccherone / node-localstorage

A drop-in substitute for the browser native localStorage API that runs on node.js.
MIT License
450 stars 36 forks source link

Proxy set handler does not return true on successful writes. #86

Closed bmcbarron closed 1 year ago

bmcbarron commented 1 year ago

If a Proxy set trap does not return true, then a javascript implementation is allowed to throw. It seems that most implementations do not, but node 18.16.1 on arm7 does.

The return path on line 78 returns the value on the right side of the assignment. When the last local storage key is removed, this.length is set to 0, which is falsish, and so this branch throws.

The return path on line 80 delegates to setItem which returns undefined on some paths, which is also falsish.

One way to fix would be to always have set return true.

bmcbarron commented 1 year ago

If you're ok with the return true solution, I can send a pull request. Although reviewing it might be more work than making the change yourself. Let me know what you'd like me to do. BTW - useful tool, thanks!

lmaccherone commented 1 year ago

@bmcbarron, I'm thinking about how best to implement this, but the conventions of this project are very out of date from my new normal. What do you think of me only supporting later versions of node.js going forward? For this project, I'm thinking of specifying node v13.2 or later so I can produce a single ESM formatted output. Thoughts?

bmcbarron commented 1 year ago

I use node v18 everywhere except Raspberry Pi, where I use node v16. So, I'm personally not concerned about that restriction. I've moved all of my code to ESM, so 👍 on that point, too.

lmaccherone commented 1 year ago

I did a quick release of v3.0.0 with your fix and a few other upgrades. While I have the hood up, I'm going to do a few more things so expect another version increment later today or tomorrow.