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

Use pure empty object as storage, to prevent key conflicts #66

Conversation

Kocal
Copy link

@Kocal Kocal commented Mar 24, 2018

For now, it is not possible to write localStorage.getItem(key) when key is equal to 'key', 'clear', ... because those keys have the same name than some of LocalStorage mock properties.

I added a failing test (because I didn't fixed it yet) which gives me this error TypeError: Cannot assign to read only property 'key' of object '[object Object]'.

@Kocal Kocal changed the title WIP: localStorage key should not be restricted Use pure empty object as storage, to prevent key conflicts Mar 24, 2018
@Kocal Kocal force-pushed the fix/use-external-object-instead-of-local-storage-itself branch from 48be6ea to c83ef76 Compare March 24, 2018 07:14
@Kocal Kocal force-pushed the fix/use-external-object-instead-of-local-storage-itself branch from c83ef76 to b5ac608 Compare March 24, 2018 07:15
@Kocal
Copy link
Author

Kocal commented Mar 24, 2018

Well, I don't know why CircleCI has not started yet, but this is what my test do:

capture d ecran de 2018-03-24 08-10-14

Then after my patch, everything is green 🎉
capture d ecran de 2018-03-24 08-12-50

@clarkbw
Copy link
Owner

clarkbw commented Mar 28, 2018

I'm not sure what's going on with CircleCI here... 🤔

@clarkbw
Copy link
Owner

clarkbw commented Mar 28, 2018

@Kocal do we have a test for iterating over the keys in localstorage? And if not can you make sure that still works with this change? Object.keys(localstorage) should equal all the items in the storage object.

@Kocal
Copy link
Author

Kocal commented Mar 28, 2018

Tests for iterating over keys in local storage... I checked quickly (I am on smartphone atm) and there is no real tests for keys iterating.

There is some tests for __STORE__ key (which is the object that contains all local storage data), but that's all. 😕

I will write a test asap I am on a pc.

@Kocal
Copy link
Author

Kocal commented Mar 28, 2018

Well, it seems that my patch fucked up Object.entries/keys/values behavior... 🤔

I will revert it and find a better solution to fix the original issue. 🤔

@clarkbw
Copy link
Owner

clarkbw commented Mar 28, 2018

I will revert it and find a better solution to fix the original issue. 🤔

🙇 Thanks for looking into this!

@clarkbw
Copy link
Owner

clarkbw commented Mar 28, 2018

If you want to commit that new iteration test as a separate PR, please feel free to send this way. 💯

Grrrr 🔥 CircleCI

@Kocal
Copy link
Author

Kocal commented Mar 29, 2018

Well, after many research and some experimentation, I was not able to reproduce the behavior of the real storage system... I can't iterate over storage data without having key conflicts. 😞

For now there is two possibilities:

  1. Close this issue and keep having key conflicts
  2. Remove iteration support and no more key conflicts

In my own opinion, I think having no key conflicts is 12871829 times more important than having iteration support, but I may be wrong.

@clarkbw
Copy link
Owner

clarkbw commented Mar 29, 2018

Hmm, I wasn't aware of this behaviour until now, it'll take some rethinking of how we do functions and storage etc.

localStorage.setItem('key', 'value')
> undefined
localStorage.key
> function key()
localStorage.getItem('key')
> "value"

@clarkbw clarkbw closed this Jan 29, 2019
@clarkbw clarkbw reopened this Jan 29, 2019
@Kocal
Copy link
Author

Kocal commented Feb 16, 2019

Hey, what should we do with this PR?

I wasn't able to a solution for keeping iteration without key-names conflict. 😕

@clarkbw
Copy link
Owner

clarkbw commented Feb 19, 2019

Ugh, I guess just waiting isn't solving this 😄

I don't want to revert the iteration that was requested and implemented previously but this does tend to create annoying errors. I think we'll have to leave this until we can find a new way to fix this and have both.

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