Fix "localStorage is not available for opaque origins" error when using mapbox-gl-js-mock #7455

fc · 2018-10-21T23:18:05Z

When running a test from jest and in the context it's being ran, it needs a URL to be set on the jsdom (it defaults to 'about:blank')...
Reference to a lengthy discussion about it: jsdom/jsdom#2304

I'll admit this is a bit of a workaround but here is no other way to set the options for jsdom and trying the suggestions to lock in a specific version of jsdom didn't work for me (and the other suggestions for changing jest didn't matter since jsdom is instantiated from this changed file).

Here is a reproduction based off create-react-app:
https://github.com/fc/mapbox-gl-js-mock-repro

Here is the test:
https://github.com/fc/mapbox-gl-js-mock-repro/blob/master/src/App.test.js

import mapboxgl from 'mapbox-gl-js-mock';

it('renders without crashing', () => {
    const map = new mapboxgl.map();
});

Run this command after a yarn install:

yarn test

Error output is:

 FAIL  src/App.test.js
  ● Test suite failed to run

    SecurityError: localStorage is not available for opaque origins

    > 1 | import mapboxgl from 'mapbox-gl-js-mock';
        | ^
      2 |
      3 | it('renders without crashing', () => {
      4 |     const map = new mapboxgl.map();

      at Window.get localStorage [as localStorage] (node_modules/jsdom/lib/jsdom/browser/Window.js:257:15)
      at Object.<anonymous>.exports.extend (node_modules/mapbox-gl/src/util/util.js:154:26)
      at extend (node_modules/mapbox-gl/src/util/window.js:61:10)
      at Object.restore (node_modules/mapbox-gl/src/util/window.js:66:18)
      at Object.require (node_modules/mapbox-gl/src/util/web_worker_transfer.js:16:21)
      at Object.require (node_modules/mapbox-gl/src/source/tile_id.js:5:20)
      at Object.require (node_modules/mapbox-gl/src/util/tile_cover.js:4:28)
      at Object.require (node_modules/mapbox-gl/src/geo/transform.js:8:17)
      at Object.require (node_modules/mapbox-gl-js-mock/classes/map.js:9:17)
      at Object.require (node_modules/mapbox-gl-js-mock/index.js:10:8)
      at Object.<anonymous> (src/App.test.js:1:1)

…ng mapbox-gl-js-mock When running a test from jest and in the context it's being ran, it needs a URL to be set on the jsdom (it defaults to 'about:blank')... Reference: jsdom/jsdom#2304 I'll admit this is a bit of a workaround but here is no other way to set the options for jsdom and trying the suggestions to lock in a specific version of jsdom didn't work for me.

ChrisLoer · 2018-10-22T20:16:12Z

@ryanhamley, could you review?

fc · 2018-10-22T22:20:38Z

This looks like it would also require an update to mapbox-gl-js-mock which uses mapbox-gl@^0.44.1...

ryanhamley · 2018-10-23T21:22:49Z

This appears to have been fixed directly in Jest so I'm inclined to close this PR and encourage anyone seeing this issue to upgrade Jest to at least version 23.5.0.

Notes on bug:
It seems like this was caused by mapbox-gl-js-mock asking for"jsdom": "^11.6.2". The caret causes new installs of JSDOM in mapbox-gl-js-mock to update to 11.12.0 which seems to be the source of this problem, if I'm understanding some of the discussion in the JSDOM and Jest repos. mapbox-gl-js has "jsdom": "11.11.0" so it won't update to 11.12; it shouldn't matter for gl-js though since the issue is that running gl-js-mock in a Node context means JSDOM doesn't have access to localStorage which isn't a problem in the browser.

Jest issue: jestjs/jest#6766
JSDOM issue: jsdom/jsdom#2304

fc · 2018-10-23T22:18:15Z

@ryanhamley thanks for the investigation...

I can understand why you would say that but the create-react-app repro is already using Jest 23.6.0.

Since mapbox-gl is using [email protected], it doesn't have the problem, the bug won't be seen but upgrading jsdom you'll run into the bug (which mapbox-gl would presumably do at some point).

After upgrading mapbox-gl to [email protected] and running the mapbox-gl unit tests:

yarn add -D [email protected]
npm run test-unit

The error will occur:

> build/run-tap --reporter classic --no-coverage test/unit

file:///Users/me/tmp/mapbox-gl-js/node_modules/jsdom/lib/jsdom/browser/Window.js:1
SecurityError: localStorage is not available for opaque origins
    at Window.get localStorage [as localStorage] (file:///Users/me/tmp/mapbox-gl-js/node_modules/jsdom/lib/jsdom/browser/Window.js:257:15)
    at extend (file:///Users/me/tmp/mapbox-gl-js/src/util/util.js:156:26)
    at restore (file:///Users/me/tmp/mapbox-gl-js/src/util/window.js:72:29)
    at Object.<anonymous> (file:///Users/me/tmp/mapbox-gl-js/src/util/window.js:15:1)
    at Generator.next (<anonymous>)
    at Object._compile (file:///Users/me/tmp/mapbox-gl-js/node_modules/pirates/lib/index.js:91:24)
    at Object.newLoader [as .js] (file:///Users/me/tmp/mapbox-gl-js/node_modules/pirates/lib/index.js:96:7)
file:///Users/me/tmp/mapbox-gl-js/node_modules/jsdom/lib/jsdom/browser/Window.js:1
SecurityError: localStorage is not available for opaque origins
    at Window.get localStorage [as localStorage] (file:///Users/me/tmp/mapbox-gl-js/node_modules/jsdom/lib/jsdom/browser/Window.js:257:15)
    at extend (file:///Users/me/tmp/mapbox-gl-js/src/util/util.js:156:26)
    at restore (file:///Users/me/tmp/mapbox-gl-js/src/util/window.js:72:29)
    at Object.<anonymous> (file:///Users/me/tmp/mapbox-gl-js/src/util/window.js:15:1)
    at Generator.next (<anonymous>)
    at Object._compile (file:///Users/me/tmp/mapbox-gl-js/node_modules/pirates/lib/index.js:91:24)
    at Object.newLoader [as .js] (file:///Users/me/tmp/mapbox-gl-js/node_modules/pirates/lib/index.js:96:7)
test/unit/mapbox-gl.js ................................ 0/1

Also I noticed the original PR would need to be updated since JSDOM is instantiated twice, not just once.

And... after applying the fix I had suggested in PR, it doesn't look like it would pass the unit tests (and perhaps why jsdom was locked in at 11.11.0 for mapbox-gl).

test/unit/style/style.test.js ..................... 159/161
  Style > registers plugin listener
  not ok expect truthy value
    at:
      line: 73
      column: 11
      file: test/unit/style/style.test.js
      type: Test
      function: t.test
    stack: |
      Test.t.test (test/unit/style/style.test.js:73:11)
      Test._4f7‍.r.test (test/unit/style/style.test.js:63:7)
      Object.<anonymous> (test/unit/style/style.test.js:57:1)
      Generator.next (<anonymous>)
      Wl (node_modules/esm/esm.js:1:207655)
      node_modules/esm/esm.js:1:208527
      Jl (node_modules/esm/esm.js:1:208833)
      Object.x (node_modules/esm/esm.js:1:244053)
      Object.o._compile (node_modules/esm/esm.js:1:210603)
      Object._compile (node_modules/pirates/lib/index.js:91:24)
      u (node_modules/esm/esm.js:1:244419)
      l (node_modules/esm/esm.js:1:243554)
      node_modules/esm/esm.js:1:241258
      Object.apply (node_modules/esm/esm.js:1:174675)
      Object.newLoader [as .js] (node_modules/pirates/lib/index.js:96:7)
      node_modules/esm/esm.js:1:216819
      node_modules/esm/esm.js:1:216854
      node_modules/esm/esm.js:1:217046
      su (node_modules/esm/esm.js:1:210621)
      Su (node_modules/esm/esm.js:1:216189)
      Eu (node_modules/esm/esm.js:1:217126)
      Function.<anonymous> (node_modules/esm/esm.js:1:251193)
      Function.<anonymous> (node_modules/esm/esm.js:1:251021)
      Function.<anonymous> (node_modules/esm/esm.js:1:241258)
      Object.apply (node_modules/esm/esm.js:1:174675)
    source: >
      t.ok(style.dispatcher.broadcast.calledWith('loadRTLTextPlugin', "some bogus
      url"));

  Style > loads plugin immediately if already registered
  not ok should be equal
    +++ found
    --- wanted
    -RTL Text Plugin failed to import scripts from /plugin.js
    +RTL Text Plugin failed to import scripts from http://localhost/plugin.js
    compare: ===

Anyway... you're right gl-js-mock is bumping jsdom. Maybe I can get gl-js-mock to lock in jsdom at 11.11.0.

PR created here: mapbox/mapbox-gl-js-mock#29

fc · 2018-10-23T22:28:53Z

If there is no intention to upgrade jsdom for mapbox-gl-js, does anyone know the maintainer of mapbox-gl-js-mock so you can walk ever there and approve that PR? The repos appears inactive with unanswered issues/PRs.
Is mapbox-gl-js-mock dead...?

ryanhamley · 2018-10-23T23:45:22Z

Updating in both places makes the error go away but introduces the problems with the RTL plugin as you note. The issue seems to be that setting the window URL changes the pluginURL from /plugin.js to http://localhost/plugin.js. It's possible to fix the tests by updating them to reflect the base URL. For example, style.test.js line 73

t.ok(style.dispatcher.broadcast.calledWith('loadRTLTextPlugin', "some bogus url"));

becomes

t.ok(style.dispatcher.broadcast.calledWith('loadRTLTextPlugin', "http://localhost/some%20bogus%20url"));

But I'm very wary of changing the library's window implementation's base URL. cc @ChrisLoer

ryanhamley · 2018-10-23T23:58:35Z

Apparently the root cause of the issue is that some code is copying JSDOM window globals onto the Node global object, as described here. No one that I've seen in the JSDOM or Jest issues has pointed to the offending code or library yet. I don't see anywhere that mapbox-gl-js is copying JSDOM properties over to global which makes me think it's some sub-dependency.

fc · 2018-10-24T00:45:26Z

Aha! It looks like you're right but it looks like mapbox-gl itself is the one doing the copying, reference:

https://github.com/mapbox/mapbox-gl-js/blob/master/src/util/window.js#L72

https://github.com/mapbox/mapbox-gl-js/blob/master/src/util/util.js#L153-L160

Relevant lines of the stack trace here:

    at Window.get localStorage [as localStorage] (file:///Users/me/tmp/mapbox-gl-js/node_modules/jsdom/lib/jsdom/browser/Window.js:257:15)
    at extend (file:///Users/me/tmp/mapbox-gl-js/src/util/util.js:156:26) <-- boom boom
    at restore (file:///Users/me/tmp/mapbox-gl-js/src/util/window.js:72:29) <-- boom

And, more specifically, localStorage is a getter on window object which is triggered by the extend call.
https://github.com/jsdom/jsdom/blob/master/lib/jsdom/browser/Window.js#L237-L243

fc · 2018-10-24T16:21:59Z

I'm trying to understand src/util/window.js...

Seeing 2 problems total, one is that localStorage is actually used in one of mapbox-gl tests here (which would trigger the localStorage getter):
https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/util/mapbox.test.js#L361

And, that it is also doing the extend on the window (which triggers the localStorage getter).

So, if you can validate this thinking...
jsdom needs both the URL set when JSDom is instantiated and it needs to not copy the window properties with extend.

A few things... it seems like we can't just do _window = window because when calling window.restore(), we want to get keep the same reference which is why it is copied/extending in the first place. The restore function even goes so far as to delete all the properties off of it at the beginning of the restore function to maintain the reference. window is treated like a global variable.

So.... it could be an acceptable trade off to only set the URL for the JSDom.

And just a note that my PR only addressed the JSDom where it is instantiated the first time (it is actually instantiated twice) so if my pr is re-opened then that'd need to be addressed along with the broken tests.

ryanhamley · 2018-10-24T20:31:10Z

So after doing some digging and research of my own, I have a better grasp of what's happening here. src/util/window.js is only used in the context of our test suite. When the library is run in the browser, Rollup replaces the default implementation of window with the browser specific version.

Interestingly, given JSDOM's explanation, I don't think the error is caused specifically by copying JSDOM properties to the Node global object, but rather by copying them to any object. The extend(_window, window) call has nothing to do with Node's global; it's just copying properties from one JSDOM instance to another. But anytime the JSDOM implementation of localStorage is copied to a new object when document.origin === null, this error is thrown.

The upshot of all of this is that as long as our tests are passing, I think adding the URL to JSDOM in that file is safe to do. I'm going to reopen the pull request. Edit: Note that you actually only need to add the URL to the second instantiation of JSDOM because it's copied over to the first one.

cc @ChrisLoer @mourner Am I missing any reason we shouldn't add a URL to the JSDOM implementation?

fc · 2018-10-24T20:52:27Z

@ryanhamley Looks like jsdom can be version bumped to the latest too (jsdom@^12.2.0). After running the unit tests, it fails on the same tests as noted earlier in this thread (test/unit/style/style.test.js).

asheemmamoowala · 2018-10-24T23:16:03Z

Am I missing any reason we shouldn't add a URL to the JSDOM implementation?

Setting url to JSDOM changes the origin header of outbound requests and brings back a new variety of the intermittent Error: Cross Origin null forbidden. Running > yarn test-unit and > yarn test-render a couple times always reproduces the issue for me.
In assigning a fixed url, the CORS issue is exacerbated in that requests need to explicitly allow the host:port combination to satisfy the CORS constraints.

ryanhamley · 2018-10-30T00:53:40Z

So a somewhat hacky solution I found is this:

    [window, _window].forEach(win => {
      delete win.localStorage;
      delete win.sessionStorage;
    })

It avoids the problems of setting a base URL for JSDOM and allows us to continue upgrading JSDOM. Anything that uses localStorage in the tests just mocks it out anyway as seen here https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/util/mapbox.test.js#L361

fc · 2018-10-30T02:43:04Z

Solid solution! It's even partly in line with the original intention of deleting the props off window at the top of restore. Thumbs up.

ryanhamley · 2018-10-30T22:45:38Z

I submitted my fix as a separate PR #7516 so I'm going to close this one. Thanks so much @fc for flagging this and helping me track down the problem.

fc mentioned this pull request Oct 22, 2018

Jest, Enzyme, Have a site using mapbox-gl-js that is failing in test coverage, pointing to mapbox-gl as culprit. #5026

Closed

ryanhamley closed this Oct 23, 2018

Add URL to JSDOM

02ed5fa

fc mentioned this pull request Oct 23, 2018

Lock in Jsdom @ 11.11.0 to prevent localStorage error mapbox/mapbox-gl-js-mock#29

Closed

ryanhamley reopened this Oct 24, 2018

ryanhamley mentioned this pull request Oct 24, 2018

Clarify that Node-specific files are replaced by Rollup in browser environments #7480

Merged

1 task

ryanhamley mentioned this pull request Oct 30, 2018

Stub local and session storage in JSDOM #7516

Merged

ryanhamley closed this Oct 30, 2018

fc mentioned this pull request Nov 19, 2018

Upgrade mapbox mapbox/mapbox-gl-js-mock#34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "localStorage is not available for opaque origins" error when using mapbox-gl-js-mock #7455

Fix "localStorage is not available for opaque origins" error when using mapbox-gl-js-mock #7455

fc commented Oct 21, 2018 •

edited

Loading

ChrisLoer commented Oct 22, 2018

fc commented Oct 22, 2018

ryanhamley commented Oct 23, 2018

fc commented Oct 23, 2018 •

edited

Loading

fc commented Oct 23, 2018 •

edited

Loading

ryanhamley commented Oct 23, 2018

ryanhamley commented Oct 23, 2018

fc commented Oct 24, 2018 •

edited

Loading

fc commented Oct 24, 2018 •

edited

Loading

ryanhamley commented Oct 24, 2018 •

edited

Loading

fc commented Oct 24, 2018

asheemmamoowala commented Oct 24, 2018

ryanhamley commented Oct 30, 2018

fc commented Oct 30, 2018 •

edited

Loading

ryanhamley commented Oct 30, 2018

Fix "localStorage is not available for opaque origins" error when using mapbox-gl-js-mock #7455

Fix "localStorage is not available for opaque origins" error when using mapbox-gl-js-mock #7455

Conversation

fc commented Oct 21, 2018 • edited Loading

ChrisLoer commented Oct 22, 2018

fc commented Oct 22, 2018

ryanhamley commented Oct 23, 2018

fc commented Oct 23, 2018 • edited Loading

fc commented Oct 23, 2018 • edited Loading

ryanhamley commented Oct 23, 2018

ryanhamley commented Oct 23, 2018

fc commented Oct 24, 2018 • edited Loading

fc commented Oct 24, 2018 • edited Loading

ryanhamley commented Oct 24, 2018 • edited Loading

fc commented Oct 24, 2018

asheemmamoowala commented Oct 24, 2018

ryanhamley commented Oct 30, 2018

fc commented Oct 30, 2018 • edited Loading

ryanhamley commented Oct 30, 2018

fc commented Oct 21, 2018 •

edited

Loading

fc commented Oct 23, 2018 •

edited

Loading

fc commented Oct 23, 2018 •

edited

Loading

fc commented Oct 24, 2018 •

edited

Loading

fc commented Oct 24, 2018 •

edited

Loading

ryanhamley commented Oct 24, 2018 •

edited

Loading

fc commented Oct 30, 2018 •

edited

Loading