panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

Missing check if Error.captureStackTrace is undefined #127

Closed Eblax closed 6 years ago

Eblax commented 6 years ago

I'm using this module in an Ionic mobile project. This works fine in Chrome and on Android, but it would not run correctly on iPhone and Firefox. I tracked the issue down to a missing check if Error.captureStackTrace is defined in lib\open_id_connect_error.js, which would cause Issuer.discover to stop running.

For a quick workaround I added if(Error && !Error['captureStackTrace']) { Error['captureStackTrace'] = function() {} } before the first call to Issuer.discover.

I see most other node modules in my project do an if (Error.captureStackTrace) before running it so maybe that could be an idea?

panva commented 6 years ago

If you're not running in a V8-based environment like node then maybe using something like https://www.npmjs.com/package/error-polyfill globally in your app might resolve all of this at once in all modules you require in addition.

I'm hesitant to put a workaround in for an environment I cannot run CI with and therefore ensure everything works as intended.

Eblax commented 6 years ago

Fair enough, I'll check that out.

panva commented 6 years ago

Let me know how it goes, if it comes down to just this and you don't hit any other issues, I'll merge the PR.

Eblax commented 6 years ago

Seems like other modules required by this one (e.g. got) have the same issue, so feel free to disregard the pull request. The error-polyfill had some issues on its own ('Unexpected frame by generating stack.') so I'll stick with my workaround for the time being. Thanks for the quick reply!

panva commented 6 years ago

👌

panva commented 6 years ago

FWIW this seems to be the polyfill you should use to actually get stack traces.

if (!Error.captureStackTrace) {
  Error.captureStackTrace = function captureStackTrace (error) {
    var container = new Error()

    Object.defineProperty(error, 'stack', {
      configurable: true,
      get: function getStack () {
        var stack = container.stack

        // Replace property with value for faster future accesses.
        Object.defineProperty(this, 'stack', {
          configurable: true,
          value: stack,
          writable: true
        })

        return stack
      },
      set: function setStack (stack) {
        Object.defineProperty(error, 'stack', {
          configurable: true,
          value: stack,
          writable: true
        })
      }
    })
  }
}

Credit: https://github.com/JsCommunity/make-error/blob/f3086a7f33ad1451e474fbcace5698275fa27eda/index.js#L12-L40

Eblax commented 6 years ago

Thanks, that seems to work like a charm (after replacing defineProperty with Object.defineProperty) I'll use that to make my workaround a bit less workaroundy :)