Check localstorage availability before accessing it #5616
Conversation
mkendall07
added
the
needs 2nd review
|
Is local storage access gated by the consent module, where applicable? If you can't read a cookie then No local storage soup for you either. |
There was a problem hiding this comment.
Can you please investigate this test failure? I have run the browserstack tests several times and this test always appears to fail in IE11.
storage manager
✗ should not throw if the localstorage is not accessible when setting/getting/removing from localstorage
TypeError: Cannot redefine non-configurable property 'localStorage'
at restore (node_modules/sinon/pkg/sinon.js:2881:13)
at Anonymous function (webpack:///test/spec/unit/core/storageManager_spec.js:60:5 <- test/test_index.js:368578:5)
Thanks
| const localstorageStub = sinon.stub(global.window, 'localStorage').get(() => { throw Error }); | ||
| const errorLogSpy = sinon.spy(utils, 'logError'); | ||
|
|
||
| coreStorage.setDataInLocalStorage('key', 'value'); | ||
| const val = coreStorage.getDataFromLocalStorage('key'); | ||
| coreStorage.removeDataFromLocalStorage('key'); | ||
|
|
||
| expect(val).to.be.null; | ||
| sinon.assert.calledThrice(errorLogSpy); | ||
|
|
||
| localstorageStub.restore(); | ||
| errorLogSpy.restore(); |
There was a problem hiding this comment.
Can you move the stubs (and the subsequent restores) into a before/after function surrounding this test? This will help ensure these objects are properly handled in case the test fails. If you don't want to have these stubs effect the other tests, please feel free to create another describe block to nest it altogether.
There was a problem hiding this comment.
Ok 👍
Btw, in this case what is "third party context"? Is that just JS loaded from 3p domain?
access window.localstorage in a third-party context
Indeed, 3rd-party context refers to scripts run inside an iframe whose src domain is not the 1p domain for instance.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@Swiiip any update on this? For me this sounds rather critical |
|
Hello sorry for the delay I'm trying to make the test pass on IE 11, the error seems linked to the way sinon mocks the localstorage property getters. |
| throw Error; | ||
| } | ||
| }; | ||
| Object.defineProperty(window, 'localStorage', mock); |
There was a problem hiding this comment.
Hi @Swiiip
Can you remove this property as part of the afterEach()? From how it looks now, this mocked function may be available for other tests in other files (since we generally run everything together).
Thanks
There was a problem hiding this comment.
Done, I reset the property descriptor after each test
Swiiip
force-pushed
the
check-localstorage-availability-before-get
branch
from
88022e5
to
cf3f014
Compare
Type of change
Description of change
Chrome now throws an error when trying to access window.localstorage in a third-party context when third-party cookies are disabled (Private mode for instance). This PR checks whether the localStorage is available each time we try to access it.