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

Refactor event propagation #148

Merged
merged 2 commits into from Sep 8, 2016
Merged

Refactor event propagation #148

merged 2 commits into from Sep 8, 2016

Conversation

@jasonpincin
Copy link
Contributor

jasonpincin commented Sep 8, 2016

This PR refactors how events are propagated to process[Symbol.for('wreck')]. Instead of attempting to link the instances of Wreck and process emitter via Object.assign, which results in broken state, this PR keeps them separate and replays emits from Wreck instances to the EventEmitter hanging off the process object.

At the bottom of this description, I'm posting a sample that shows the broken state resulting from the current implementation.

This PR fixes #145 and joyent/node-consulite#3.

Here's the simplest code I could think of to demonstrate the current disconnected state between process and Wreck emitters:

'use strict'
const hoek = require('hoek')
const EventEmitter = require('events').EventEmitter

const emitter = new EventEmitter

function myEmitter () {
    Object.assign(this, emitter)
}

hoek.inherits(myEmitter, EventEmitter)

const emitterClone = new myEmitter()

emitterClone.on('request', requestHandler)
console.log('after add')
console.log('emitter handlers\n------------\n', emitter._events, '\n')
console.log('emitterClone handlers\n------------\n', emitterClone._events)

console.log('\n\n')

emitterClone.removeListener('request', requestHandler)
console.log('after remove')
console.log('emitter handlers\n------------\n', emitter._events, '\n')
console.log('emitterClone handlers\n------------\n', emitterClone._events)

function requestHandler () {}
@jasonpincin

This comment has been minimized.

Copy link
Contributor Author

jasonpincin commented Sep 8, 2016

@geek I closed #146 in favor of this. I'm suggesting this as the actual fix.

@geek

This comment has been minimized.

Copy link
Member

geek commented Sep 8, 2016

This looks good, I like this approach better than the current one. I am going to mark it as a breaking change because it will change the behavior if you were using multiple instances of wreck with the events.

    it('wreck instances share the same emitter', (done) => {

        const server = Http.createServer((req, res) => {

            res.writeHead(200);
            res.end('ok');
        });

        const wreck1 = Wreck.defaults({});
        const wreck2 = Wreck.defaults({});

        let handled = 0;
        wreck1.once('request', () => {

            handled++;
        });

        wreck2.once('request', () => {

            handled++;
        });

        server.listen(0, () => {

            wreck1.get('http://localhost:' + server.address().port, (err, res, payload) => {

                expect(err).to.not.exist();
                expect(res.statusCode).to.equal(200);
                expect(payload.toString()).to.equal('ok');
                expect(handled).to.equal(2);
                server.close();
                done();
            });
        });
    });
@geek geek added this to the 10.0.0 milestone Sep 8, 2016
@geek geek self-assigned this Sep 8, 2016
@geek geek merged commit 93a181d into hapijs:master Sep 8, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek geek added bug and removed feature labels Sep 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.