Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: propagate errors to single-response APIs #277

Merged
merged 5 commits into from Apr 9, 2019
@@ -2,7 +2,7 @@ import jsonrpc from './jsonrpc'
import MessagePortMessage from './providers/MessagePortMessage'
import WindowMessage from './providers/WindowMessage'
import DevMessage from './providers/DevMessage'
import { first, filter } from 'rxjs/operators'
import { first, filter, map } from 'rxjs/operators'

export const providers = {
MessagePortMessage,
@@ -91,7 +91,14 @@ export default class Messenger {
const id = this.send(method, params)

return this.responses().pipe(
filter((message) => message.id === id)
filter((message) => message.id === id),
map((response) => {
if (response.error) {

This comment has been minimized.

Copy link
@2color

2color Apr 9, 2019

Contributor

Is there any reason not to leave the response.error an error instance in https://github.com/aragon/aragon.js/blob/master/packages/aragon-rpc-messenger/src/jsonrpc.js#L19?

It seems like we are converting an error to a string in jsonrpc.js and back to an error here.

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 9, 2019

Author Member

Primarily because sending an Error instance through postMessage() will fail: see things that don't work with the structured clone algorithm.

response.error = new Error(response.error)
}
return response
})
// Let callers handle errors themselves
)
}

@@ -104,7 +111,14 @@ export default class Messenger {
*/
sendAndObserveResponse (method, params = []) {
return this.sendAndObserveResponses(method, params).pipe(
first()
first(),
map((response) => {
// Emit an error if the response is an error
if (response.error) {
throw response.error
}
return response
})
)
}
}
@@ -69,13 +69,14 @@ test('should encode and send the request', t => {
})

test('should filter the incoming messages to responses only', (t) => {
t.plan(2)

// arrange
const busMock = new Subject()
const instance = new Messenger(null)
instance.bus = () => busMock
jsonrpcStub.isValidResponse = sinon.stub().returns(true)
// assert
t.plan(2)

// act
const messages = instance.responses()
messages.subscribe(value => {
@@ -87,13 +88,14 @@ test('should filter the incoming messages to responses only', (t) => {
})

test('should filter the incoming messages to requests only', (t) => {
t.plan(2)

// arrange
const busMock = new Subject()
const instance = new Messenger(null)
instance.bus = () => busMock
jsonrpcStub.isValidResponse = sinon.stub().returns(false)
// assert
t.plan(2)

// act
const messages = instance.requests()
messages.subscribe(value => {
@@ -105,38 +107,89 @@ test('should filter the incoming messages to requests only', (t) => {
})

test('should send and observe responses', (t) => {
t.plan(4)

// arrange
const instance = new Messenger(null)
sinon.stub(instance, 'send').returns(41)
sinon.stub(instance, 'responses').returns(new Subject())
const messages = instance.sendAndObserveResponses('sendEth')

// assert
t.plan(4)
messages.subscribe(value => t.is(value.data, 'thanks'))
t.is(instance.send.getCall(0).args[0], 'sendEth')
t.deepEqual(instance.send.getCall(0).args[1], [])

// act
instance.responses().next({ data: 'thanks', id: 41 })
instance.responses().next({ data: 'thanks', id: 41 })
})

test('should send and observe responses, even if errors are included', (t) => {
t.plan(6)

// arrange
const instance = new Messenger(null)
sinon.stub(instance, 'send').returns(41)
sinon.stub(instance, 'responses').returns(new Subject())
const messages = instance.sendAndObserveResponses('sendEth')
messages.subscribe(value => t.is(value.data, 'thanks'))

// assert
messages.subscribe(value => {
if (value.data) {
t.is(value.data, 'thanks')
} else if (value.error) {
t.true(value.error instanceof Error)
t.is(value.error.message, 'no thanks')
}
})
t.is(instance.send.getCall(0).args[0], 'sendEth')
t.deepEqual(instance.send.getCall(0).args[1], [])

// act
instance.responses().next({ data: 'thanks', id: 41 })
instance.responses().next({ error: 'no thanks', id: 41 })
instance.responses().next({ data: 'thanks', id: 41 })
})

test('should send and observe only the first response', (t) => {
t.plan(3)

// arrange
const instance = new Messenger(null)
sinon.stub(instance, 'sendAndObserveResponses').returns(new Subject())
// assert
t.plan(3)

// act
const messages = instance.sendAndObserveResponse('sendAnt')
// assert
messages.subscribe(value => t.is(value, 'first'))

t.is(instance.sendAndObserveResponses.getCall(0).args[0], 'sendAnt')
t.deepEqual(instance.sendAndObserveResponses.getCall(0).args[1], [])

instance.sendAndObserveResponses().next('first')
instance.sendAndObserveResponses().next('second')
instance.sendAndObserveResponses().next('third')
})

test('should send and observe only the first error', (t) => {
t.plan(4)

// arrange
const instance = new Messenger(null)
sinon.stub(instance, 'sendAndObserveResponses').returns(new Subject())
const messages = instance.sendAndObserveResponse('sendAnt')
// assert
messages.subscribe(
value => t.fail('should not have emitted any next values'),
error => {
t.true(error instanceof Error)
t.is(error.message, 'bad first')
}
)
t.is(instance.sendAndObserveResponses.getCall(0).args[0], 'sendAnt')
t.deepEqual(instance.sendAndObserveResponses.getCall(0).args[1], [])

// act
instance.sendAndObserveResponses().next({ error: new Error('bad first') })
instance.sendAndObserveResponses().next('second')
})
@@ -6,8 +6,8 @@ export function createResponse ({ request: { id } }, { error, value = null, kind
return {}
}

if (error) {
return { id, payload: error }
if (kind === 'E') {
return { id, payload: error || new Error() }
}

return { id, payload: value }
@@ -34,7 +34,7 @@ export function createRequestHandler (request$, requestType, handler) {
createResponse
),
// filter empty responses caused by Notifications of kind 'C'
filter((response) => response.payload !== undefined || response.error !== undefined)
filter((response) => response.payload !== undefined)
)
}

@@ -12,7 +12,7 @@ test.afterEach.always(() => {
})

test('should create a request handler', async (t) => {
t.plan(4)
t.plan(5)
// arrange
const requestStub = from([{
request: {
@@ -33,6 +33,12 @@ test('should create a request handler', async (t) => {
params: ['set', 'settings'],
value: { foo: 'bar' }
}
}, {
request: {
id: 'uuid5',
method: 'cache',
params: ['clear']
}
}, {
request: {
id: 'uuid6',
@@ -56,14 +62,21 @@ test('should create a request handler', async (t) => {
return Promise.reject(new Error(`no permissions to change ${request.params[1]}!!`))
}

if (request.params[0] === 'clear') {
return Promise.reject(new Error())
}

return Promise.resolve(`resolved ${request.params[1]}`)
}
// act
const result = createRequestHandler(requestStub, 'cache', handlerStub)
// assert
result.subscribe(value => {
if (value.id === 'uuid1') return t.is(value.payload, 'resolved settings')
if (value.id === 'uuid4') return t.true(value.payload instanceof Error)
if (value.id === 'uuid4') return t.is(value.payload.message, 'no permissions to change settings!!')
if (value.id === 'uuid5') return t.true(value.payload instanceof Error)
if (value.id === 'uuid5') return t.is(value.payload.message, '')
if (value.id === 'uuid6') return t.is(value.payload, 'resolved profile')
if (value.id === 'uuid8') return t.is(value.payload, null)
})
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.