Companion: make emitSuccess and emitError private

[mifi]

also make method private

pulled out from #3791

[goto-bus-stop]

We shouldn't make breaking changes between Uppy/Companion communication unless we have a very good reason for it. Ideally Companion should support old Uppy versions for many years.

I think extraData is also used for XHR error reporting, it will send the response to the client so you can use the XHRUpload getResponseError to handle errors the same way for local and Companion uploads.

[Murderlon]

We shouldn't make breaking changes between Uppy/Companion communication unless we have a very good reason for it. Ideally Companion should support old Uppy versions for many years.

How is this extraData currently used? How long has it been considered deprecated?

[goto-bus-stop]

I haven't heard of extraData being deprecated at all, but i haven't paid that close attention either. The biggest problem i see here is that the error is no longer nested under an error key. But I updated my comment with how extraData (used to be?)/is used.

[goto-bus-stop]

payload.response:

uppy/packages/@uppy/xhr-upload/src/index.js

Lines 428 to 433 in ef8c5cc

	socket.on('error', (errData) => {
	const resp = errData.response
	const error = resp
	? opts.getResponseError(resp.responseText, resp)
	: Object.assign(new Error(errData.error.message), { cause: errData.error })
	this.uppy.emit('upload-error', file, error)

[mifi]

I just found it a bit dirty that in the existing code, payload gets set to err.extraData, and then payload also gets an error property set on it, equal to err, which also contains extraData. And if extraData contains an error property, it will be overwritten.

const err = { message: 'test', extraData: { response: 'abc', error: 'something' } };
const { stack, ...serializedErr } = err;
console.log(JSON.stringify({ payload: { ...err.extraData, error: serializedErr } }, null, 2))

{
  "payload": {
    "response": "abc",
    "error": {
      "message": "test",
      "extraData": {
        "response": "abc",
        "error": "something"
      }
    }
  }
}

We can see that response gets duplicated, and error gets overwritten. That's why I wanted to clean up this. But I agree that it's not worth breaking the interface between companion and uppy client, which I didn't realize would happen. I have reverted the breaking change now.

[goto-bus-stop]

The client doesn't use the nested payload.error.extraData, and the fact that it exists is definitely not intended, so it would be nice to clean that up. But we should be careful even about that since error objects may be exposed to users.

At some point we may have to consider ways to upgrade the HTTP/socket APIs with backwards compat, so newer clients can opt into newer implementations and old clients still work. This case is too small to justify that work I think.

[mifi]

Ok, I'll re-add the TODO then, hopefully in the future we can fix it