{"action":"created","issue":{"url":"https://api.github.com/repos/tmpvar/jsdom/issues/1159","labels_url":"https://api.github.com/repos/tmpvar/jsdom/issues/1159/labels{/name}","comments_url":"https://api.github.com/repos/tmpvar/jsdom/issues/1159/comments","events_url":"https://api.github.com/repos/tmpvar/jsdom/issues/1159/events","html_url":"https://github.com/tmpvar/jsdom/pull/1159","id":93105350,"number":1159,"title":"Better errors","user":{"login":"domenic","id":617481,"avatar_url":"https://avatars.githubusercontent.com/u/617481?v=3","gravatar_id":"","url":"https://api.github.com/users/domenic","html_url":"https://github.com/domenic","followers_url":"https://api.github.com/users/domenic/followers","following_url":"https://api.github.com/users/domenic/following{/other_user}","gists_url":"https://api.github.com/users/domenic/gists{/gist_id}","starred_url":"https://api.github.com/users/domenic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/domenic/subscriptions","organizations_url":"https://api.github.com/users/domenic/orgs","repos_url":"https://api.github.com/users/domenic/repos","events_url":"https://api.github.com/users/domenic/events{/privacy}","received_events_url":"https://api.github.com/users/domenic/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-07-05T11:48:34Z","updated_at":"2015-07-07T09:15:40Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/tmpvar/jsdom/pulls/1159","html_url":"https://github.com/tmpvar/jsdom/pull/1159","diff_url":"https://github.com/tmpvar/jsdom/pull/1159.diff","patch_url":"https://github.com/tmpvar/jsdom/pull/1159.patch"},"body":" This PR is meant to mainly address #1039 and #587. It implements `window.onerror` (including its funky parameter stuff) and `ErrorEvent`, then uses that to catch all script errors, instead of using our current system of a non-standard `raise` method that sends things to `node.errors` or `document.errors` or whatever.\r\n\r\nIt then proceeds to use the virtual console for all other error cases currently handled by raise. This is done via a new virtual console event, `jsdomError`. In sum, the virtual console now receives `jsdomError`s in the following cases:\r\n\r\n- Errors loading scripts when using `jsdom.env`'s `scripts` array\r\n- Script execution errors that are not handled by a window `onerror` event handler that returns `true` or calls `event.preventDefault()`\r\n- Calls to methods, like `window.alert`, which jsdom does not implement, but installs anyway for web compatibility.\r\n\r\nFinally, since script errors are now handled in this way, they are removed from the initialization flow. This means that the callbacks for `env` now take a single error object, instead of an array of errors; hooray!! This part is definitely backward-incompatible, although some of the rest is probably too.\r\n\r\nBesides code review, I'd really appreciate comments on some of the details of this virtual console stuff. /ccing @jeffcarp for that purpose in particular, but any jsdom users are welcome. Here are some of my questions.\r\n\r\n- Is one event, jsdomError, good enough? Or should we split this into one event per way that it's used? (See https://github.com/tmpvar/jsdom/commit/279b21d2fe362bc69bfcd98e11d3175b948945b3#diff-04c6e90faac2675aa89e2176d2eec7d8R451) If we don't split it into multiple events, should we perhaps add a .code property or something?\r\n- What do you think of the use of Error objects with .detail properties?\r\n- Should we use this mechanism for all resource loading errors, and not just jsdom.env script loading? I tend to think \"yes.\"\r\n- Should we forward these errors to nonVirtualConsole.error or similar, when doing sendTo?\r\n- Will this new setup, especially for jsdom.env, \"hide\" errors that previously were easy to spot? Not everyone sets up a virtual console... Maybe we should store jsdomErrors at least in the virtual console for later inspection?\r\n\r\n(Re: Travis: the build passes locally; presumably it's the browser build that's broken. I'll look into that soon.)"},"comment":{"url":"https://api.github.com/repos/tmpvar/jsdom/issues/comments/119133978","html_url":"https://github.com/tmpvar/jsdom/pull/1159#issuecomment-119133978","issue_url":"https://api.github.com/repos/tmpvar/jsdom/issues/1159","id":119133978,"user":{"login":"jeffcarp","id":916028,"avatar_url":"https://avatars.githubusercontent.com/u/916028?v=3","gravatar_id":"","url":"https://api.github.com/users/jeffcarp","html_url":"https://github.com/jeffcarp","followers_url":"https://api.github.com/users/jeffcarp/followers","following_url":"https://api.github.com/users/jeffcarp/following{/other_user}","gists_url":"https://api.github.com/users/jeffcarp/gists{/gist_id}","starred_url":"https://api.github.com/users/jeffcarp/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jeffcarp/subscriptions","organizations_url":"https://api.github.com/users/jeffcarp/orgs","repos_url":"https://api.github.com/users/jeffcarp/repos","events_url":"https://api.github.com/users/jeffcarp/events{/privacy}","received_events_url":"https://api.github.com/users/jeffcarp/received_events","type":"User","site_admin":false},"created_at":"2015-07-07T09:15:40Z","updated_at":"2015-07-07T09:15:40Z","body":"Cool! Looks like a great improvement! :sunglasses:\r\n\r\ntl;dr: Better understanding the way people are using the virtual console (or not using it) might give the best answers to these questions.\r\n\r\nThoughts on specific points:\r\n\r\n> Is one event, jsdomError, good enough? Or should we split this into one event per way that it's used? (See 279b21d#diff-04c6e90faac2675aa89e2176d2eec7d8R451) If we don't split it into multiple events, should we perhaps add a .code property or something?\r\n\r\nI'd advocate for keeping it one event. The ability to add one handler and be confident you're catching all possible jsdom errors sounds like the most useful option. If you have different types of errors that you have to listen for and jsdom adds a new type of error, you'll miss those errors until you add listeners for them. \r\n\r\nAdding a `.code` property may or may not be useful based on whether `jsdomError` will be consumed more by machines or by people.\r\n\r\n> What do you think of the use of Error objects with .detail properties?\r\n\r\nI'm not sure I understand this completely. The `.detail` property is the original Error object?\r\n\r\n> Should we use this mechanism for all resource loading errors, and not just jsdom.env script loading? I tend to think \"yes.\"\r\n\r\nI'm :+1: on this. It seems like this follows what browsers do.\r\n\r\n> Should we forward these errors to nonVirtualConsole.error or similar, when doing sendTo?\r\n\r\nThis sounds slightly magical to me, but I can't think of a better alternative.\r\n\r\nThis is kind of a crazy idea, but is there a strong case against simply using `VirtualConsole.error` instead of `VirtualConsole.jsdomError` entirely? That would get rid of this issue.\r\n\r\n> Will this new setup, especially for jsdom.env, \"hide\" errors that previously were easy to spot? Not everyone sets up a virtual console... Maybe we should store jsdomErrors at least in the virtual console for later inspection?\r\n\r\nYes, the README currently does a good job of explaining the new way error handling works, but I foresee there still being some confusion if some errors are moved to the virtual console.\r\n\r\nAfter giving it some thought, the new way seems more similar to the way errors would happen in browsers. You might get an indication from the page that something wasn't right, but you'd need to pop open the console to see the actual error. Also I could be :+1: on storing errors for later consumption, but can't think of a ton of use cases right now, so possibly YAGNI."}}