Remove use of electron.remote

[Nantris]

There's a few things that remain issues here.

1. The API has been letting users pass a custom logger in the options, but functions are non-serializable. Currently I have it configured to log locally before sending the error to main.
2. Setting 'showDialog` from the renderer will not have the desired effect.
3. I'm not sure what the purpose of logError is. Just an explicit method a user can use to log an error, leveraging the configuration of electron-unhandled?

Fixes #22

[sindresorhus]

I think a solution could be to just send all errors from the renderer to the main process and handle them all there. - #22

I haven't reviewed the code yet, but make sure you do this.

Because your above questions become moot when this is done. When used in the renderer, it would not accept any options. It would be configured in the main process.

[Nantris]

When used in the renderer, it would not accept any options

Good point. The PR does send the errors to main. With that in mind I undid a small tweak I'd made where I was trying to keep the old pattern of also accepting options in renderer.

I think it should be good to go now whenever you find the time to review.

[Nantris]

@sindresorhus one remaining issue is to address stripping out entities that can't be sent via the structured clone algorithm.

I'm not sure what the best way to do that would be - perhaps you already have some solution in mind?

[sindresorhus]

I'm not sure what the best way to do that would be - perhaps you already have some solution in mind?

What happens if a type is not supported? If you pass a value directly, it probably throws, but what if it's a property. I think v8.serialize() ignores the property, but not sure.

Relevant: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

[sindresorhus]

Never bump in a PR. That's up to the maintainers to do.

[Nantris]

That's an oops from working across too many projects at once.

[Nantris]

Do you want me to submit a PR to reverse this or is it trivial enough to resolve when you determine the appropriate version for these changes?

[Nantris]

Here are the outcomes of trying to send BrowserWindow across IPC in various ways (and yeah this is terrible security but it's DEV only.)

I'm not sure what outcome the main process sees in the final example because there's nothing handling for this IPC channel but I can find out if we need to.

[sindresorhus]

Here are the outcomes of trying to send BrowserWindow across IPC in various ways (and yeah this is terrible security but it's DEV only.)

Does the error have any .code property we could check?

I think the best way might be to just try/catch the IPC call, if it throws, check if it's because it couldn't be cloned, if so, try sending v8.serialize on each property of the error and remove the properties that fail to serialize. I'm open to better ideas, but this is the best I can think of right now.

[sindresorhus]

Bump

[Nantris]

Thanks for the bumb and sorry for the long delay on this. I can't believe it's been a month already, jeez. The priority level got nudged down on our end, but it's still on my radar.

Unfortunately there's no error.code value. How are you envisioning we check for a cloning issue? Just like this?

try {
  return ipcRenderer.invoke(ERROR_HANDLER_CHANNEL, ...args);
 } catch (invokeErr) {
   if (invokeErr.message === 'An object could not be cloned.') {
      //  v8.serialize stuff
   }
}

I'd hope that the error.message would be stable across versions of V8.

try sending v8.serialize on each property of the error and remove the properties that fail to serialize

This is probably a stupid question, but how can we iterate over the properties of an error object? Object.entries(err) just yields an empty array.

---

We also need some new code for the example.js - to test/demo the invocation of the error handler in the main process from the renderer. The current example only invokes errors locally on the main process - never including the renderer-side logic.

I'm thinking we add a preload script like:

const { logError } = require('./')
logError(Symbol()); // Symbols cannot be sent with the structured clone algorithm

[sindresorhus]

How are you envisioning we check for a cloning issue? Just like this?

Could .invoke throw for any other reason? Do we even need to check the message?

This is probably a stupid question, but how can we iterate over the properties of an error object? Object.entries(err) just yields an empty array.

The standard error properties are not enumerable for some dumb reason. So you need to manually get .name, .message, and .stack.

[sindresorhus]

Friendly bump :)

[Nantris]

Friendly thank you! Too easy to take on too many projects.

I don't think that we have to check that the message is equal to An object could not be cloned but I just don't know.

Because invokeErr gives no clues about what might have failed beyond being unable to serialize, I'm not really sure how to go about sanitizing (serializing) all the fields of whatever a user passes to invokeErrorHandler, since it could really be anything as long as logError is exposed in module.exports; they could pass a Symbol a DOM node, anything, right?

If we could be sure the value would be an error it would be pretty trivial though I think - or maybe I'm just overthinking this.

So how about code like to cludge the values into an error format?

const {serialize} = require('v8');
const tryMakeSerialized = arg => {
  try {
    const serialized = serialize(arg);
    if (serialized) return serialized;
  } catch {}
}

// ....


invokeErrorHandler = async (title, error) => {
 try {
  await ipcRenderer.invoke(ERROR_HANDLER_CHANNEL, title, error);
  return;
 } catch (invokeErr) {
  if (invokeErr.message === 'An object could not be cloned.') {
    error = ensureError(error)

    const serializable = {
      name: tryMakeSerialized(error.name),
      message: tryMakeSerialized(error.message),
      stack: tryMakeSerialized(error.stack),
    };

    ipcRenderer.invoke(ERROR_HANDLER_CHANNEL, title, serializable); // Invoke the error handler again with only the serializable values
   }
 }
}

[sindresorhus]

Yeah, let's try that and we can see how it works in practice.

[Nantris]

@sindresorhus we should be good to go - but because of the IPC back and forth, different contexts, and error-inception, this has been kind of hard to reason about and I can't guarantee these changes are entirely free of mistake.

That said, I've been using our branch in development for the past 6 months or so without issue - although of course I just pushed some new commits that do change things a bit, and our use case will never trigger the structured clone/serialization failure in any event.

I deemed this line to be removable without affecting functionality testing as any error invoked via the renderer process should inherently test the main process, which seems to have been the purpose.

Also, it shouldn't be relevant since you'll be bumping the version anyway when ready to release - but a reminder that I accidentally bumped the version of electron-unhandled in one of these commits.

Let me know if you see anything that needs to be addressed!

[sindresorhus]

This still needs docs updates. For example, that unhandled() has to be called in either both renderer and main, or only main, but not just renderer process. And that the showDialog option cannot be set from the renderer process.

[sindresorhus]

And CI is failing.

[sindresorhus]

You also have a weird change to screenshot.png. Look at the diff.

[Nantris]

I had found it strange that the project didn't use ESLint. I now see that eslint_d had just stopped for me. I'll try to get the remaining issues addressed in a few weeks when crunch-time is over, before the next round of crunch-time begins.

[sindresorhus]

Bump

[Nantris]

Thanks for the bump @sindresorhus. Got COVID, so things got set back even more. Working on this now though.

the showDialog option cannot be set from the renderer process.

I think all options can only be configured from the main process now. Am I mistaken? Post-COVID brain fog is brutal, but slowly resolving thankfully.

The options object will only be accessible for updating in the main process as far as I can tell.

electron-unhandled/index.js

Lines 76 to 79 in 10680d8

	options = {
	...options,
	...inputOptions
	};

---

Edit: Also, I suppose these lines are now useless:

electron-unhandled/index.js

Lines 147 to 149 in c452122

	options = {
	...options
	};

[Nantris]

@sindresorhus can you approve the workflow checks to run so I can see what had been failing?

I think it was just linting issues, like missing semicolons, but without something like a .eslintrc I don't know what other rules are supposed to be followed.

[Nantris]

@sindresorhus I believe this is good to go besides my pending review above regarding the name of the error variable in the catch block. The only other failure in xo is a TODO that predates this PR.

[sindresorhus]

Thanks :)