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

Implement Web storage - localStorage, sessionStorage, StorageEvent #2076

Merged
merged 2 commits into from Jun 18, 2018

Conversation

Projects
None yet
6 participants
@Zirro
Member

Zirro commented Dec 7, 2017

This is a work-in-progress implementation of the Storage interface, along with localStorage, sessionStorage and the storageEvent. It resolves #1137.

All the tests for storing data are currently passing, but some of the event tests will require further work. I welcome feedback at this stage, but keep in mind that this is not yet complete.

(Also, thank you @TimothyGu for making this a breeze to implement through all your improvements to webidl2js!)

Show outdated Hide outdated lib/api.js
@domenic

Very exciting :)

Show outdated Hide outdated lib/api.js
Show outdated Hide outdated lib/jsdom/living/webstorage/Storage-impl.js
Show outdated Hide outdated lib/jsdom/living/webstorage/Storage-impl.js
@Zirro

This comment has been minimized.

Show comment
Hide comment
@Zirro

Zirro Dec 9, 2017

Member

Updated to remove the writeToDisk-ability until we've settled on an API, and use string concatenation rather than stringification to determine the size of the data being stored. Will address _taskQueue when I work on the event issues later.

Member

Zirro commented Dec 9, 2017

Updated to remove the writeToDisk-ability until we've settled on an API, and use string concatenation rather than stringification to determine the size of the data being stored. Will address _taskQueue when I work on the event issues later.

@Zirro

This comment has been minimized.

Show comment
Hide comment
@Zirro

Zirro Dec 10, 2017

Member

This is now ready for review 🎉

The only part I've knowingly not implemented is the url property for StorageEvent. It is supposed to be the URL for the context from where the Storage object was manipulated. Since an <iframe> can share the same Storage object as its parent I don't see a clear way to distinguish which context was the cause of the change. I'd appreciate suggestions for how to handle it, but otherwise I think it would be okay to place this property behind a comment in the IDL for now.

EDIT: I just had a careful read through the spec and noticed that I've overlooked a detail about separate objects. I'll have another go at completing this next week.

It would appear that one of the tests is using Object.values() which isn't available in Node.js v6, thus causing CI to fail. I'll put it behind the needs-node8 reason for my next push.

Member

Zirro commented Dec 10, 2017

This is now ready for review 🎉

The only part I've knowingly not implemented is the url property for StorageEvent. It is supposed to be the URL for the context from where the Storage object was manipulated. Since an <iframe> can share the same Storage object as its parent I don't see a clear way to distinguish which context was the cause of the change. I'd appreciate suggestions for how to handle it, but otherwise I think it would be okay to place this property behind a comment in the IDL for now.

EDIT: I just had a careful read through the spec and noticed that I've overlooked a detail about separate objects. I'll have another go at completing this next week.

It would appear that one of the tests is using Object.values() which isn't available in Node.js v6, thus causing CI to fail. I'll put it behind the needs-node8 reason for my next push.

@Zirro

This comment has been minimized.

Show comment
Hide comment
@Zirro

Zirro Dec 11, 2017

Member

It doesn't make for the prettiest code, but I think I've finally nailed the logic for storage areas with separate objects. This also made it possible to implement the url property, allowing the last relevant tests to pass :-)

Member

Zirro commented Dec 11, 2017

It doesn't make for the prettiest code, but I think I've finally nailed the logic for storage areas with separate objects. This also made it possible to implement the url property, allowing the last relevant tests to pass :-)

@Zirro

This comment has been minimized.

Show comment
Hide comment
@Zirro

Zirro Dec 13, 2017

Member

Updated to add condition for opaque origins to sessionStorage as well, in anticipation of the spec being updated for whatwg/html#3283.

Member

Zirro commented Dec 13, 2017

Updated to add condition for opaque origins to sessionStorage as well, in anticipation of the spec being updated for whatwg/html#3283.

@Zirro Zirro changed the title from [WIP] Implement Web storage - localStorage, sessionStorage, StorageEvent to Implement Web storage - localStorage, sessionStorage, StorageEvent Dec 13, 2017

@Zirro

This comment has been minimized.

Show comment
Hide comment
@Zirro

Zirro Dec 19, 2017

Member

I had time to do some further testing of my own today and found more cases not covered by the existing tests. These include making sure that data is shared between a parent and several iframes within the same origin, as well as checking that the StorageEvent is dispatched on all window's except the one through which the storage object was manipulated.

I've fixed the former issue, getting some neat refactoring done in the process. The latter issue is only partially resolved at the moment - manipulations done on a parent window will no longer cause the StorageEvent to be dispatched there, but manipulating a storage object in a frame will still result in a StorageEvent on the window of both frame and parent. More work needed there.

Considering this and some gaps in the test coverage I found earlier I'm now hesitant to merge this without writing several new tests, so this could take a while longer to complete.

Member

Zirro commented Dec 19, 2017

I had time to do some further testing of my own today and found more cases not covered by the existing tests. These include making sure that data is shared between a parent and several iframes within the same origin, as well as checking that the StorageEvent is dispatched on all window's except the one through which the storage object was manipulated.

I've fixed the former issue, getting some neat refactoring done in the process. The latter issue is only partially resolved at the moment - manipulations done on a parent window will no longer cause the StorageEvent to be dispatched there, but manipulating a storage object in a frame will still result in a StorageEvent on the window of both frame and parent. More work needed there.

Considering this and some gaps in the test coverage I found earlier I'm now hesitant to merge this without writing several new tests, so this could take a while longer to complete.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 12, 2018

Member

So I believe in IRC we decided to rebase this even without the full coverage. Maybe a good intermediate step would be to file an issue on the WPT repo detailing the missing coverage and your ideas of how you might test it?

Member

domenic commented Jun 12, 2018

So I believe in IRC we decided to rebase this even without the full coverage. Maybe a good intermediate step would be to file an issue on the WPT repo detailing the missing coverage and your ideas of how you might test it?

@Zirro

This comment has been minimized.

Show comment
Hide comment
@Zirro

Zirro Jun 14, 2018

Member

I've rebased this to take the latest commits into account. Will file an issue in the WPT repo if I have time this weekend.

Member

Zirro commented Jun 14, 2018

I've rebased this to take the latest commits into account. Will file an issue in the WPT repo if I have time this weekend.

@domenic domenic merged commit d6f8a97 into jsdom:master Jun 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
this._type = type;
// The spec suggests a default storage quota of 5 MB
this._quota = 5000;

This comment has been minimized.

@fstoerkle

fstoerkle Jun 18, 2018

@Zirro
As I understand, 5000 would suggest that the default storage quota is 5 KB... shouldn't that be 5_000_000 bytes?

@fstoerkle

fstoerkle Jun 18, 2018

@Zirro
As I understand, 5000 would suggest that the default storage quota is 5 KB... shouldn't that be 5_000_000 bytes?

@yogeshthanvi

This comment has been minimized.

Show comment
Hide comment
@yogeshthanvi

yogeshthanvi Jul 27, 2018

I am facing the below issue anyone can help me out
SecurityError: localStorage is not available for opaque origins

  at Window.get localStorage [as localStorage] (../node_modules/jsdom/lib/jsdom/browser/Window.js:257:15)
      at Array.forEach (<anonymous>)

yogeshthanvi commented Jul 27, 2018

I am facing the below issue anyone can help me out
SecurityError: localStorage is not available for opaque origins

  at Window.get localStorage [as localStorage] (../node_modules/jsdom/lib/jsdom/browser/Window.js:257:15)
      at Array.forEach (<anonymous>)
@Zirro

This comment has been minimized.

Show comment
Hide comment
@Zirro
Member

Zirro commented Jul 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment