Skip to content

Conversation

@ryanhamley
Copy link
Contributor

@ryanhamley ryanhamley commented Oct 30, 2018

Launch Checklist

This replaces #7455

JSDOM 11.12 introduced the localStorage API. Our codebase copies properties from one JSDOM instance to another which worked fine but when accessing localStorage in a Node context where the JSDOM's base URL is not set, an error is thrown. localStorage is not used in our tests (we stub it out in a couple of places) so I think this is probably an acceptable workaround. Setting the JSDOM's base URL can cause issues with some tests and creates potential CORS errors that makes testing a pain.

Tip of the 🎩 to @fc for reporting the original bug and helping figure out what exactly was going wrong.

@fc
Copy link
Contributor

fc commented Oct 30, 2018

Would it be desirable to upgrade JSDom along with this?

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanhamley Would a stub implementation with an exception be better than deleting the properties? It would help debug if new code relied on either of these.

@ryanhamley
Copy link
Contributor Author

ryanhamley commented Oct 31, 2018

I've updated package.json so JSDOM can freely upgrade and stubbed out [local/session]Storage with a warning log in case anyone attempts to access either property. Edit for posterity: The properties have to be deleted before they can be stubbed because they only have getter methods and cannot be directly overridden.

@ryanhamley ryanhamley changed the title Delete local and session storage from JSDOM Stub local and session storage in JSDOM Oct 31, 2018
@ryanhamley ryanhamley merged commit 5b5f74c into master Oct 31, 2018
@ryanhamley ryanhamley deleted the jsdom-local-storage branch October 31, 2018 00:17
@ryanhamley ryanhamley mentioned this pull request Oct 31, 2018
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.

3 participants