1

I'm creating a "class" that emits events such as error, data, downloadFile and initialize. Each event is fired after a request is made, and each event is fired by a method that has the same name:

class MyClass extends EventEmitter {
  constructor(data) {
    this.data = data

    this.initialize()
      .then(this.downloadFile)
      .then(this.data)
      .catch(this.error)
  }

  initialize() {
    const req = superagent.post('url...')
    superagent.send(data)
    const res = await req // this will actually fire the request
    this.emit('initialize')
    this.url = res.body
    return res
  }

  downloadFile() {
    const req = superagent.put(this.url)
    const res = await req; // this will actually fire the request
    req.on('progress', (progress) => this.emit('downloadFile', progress)
    //
    // save to disk
    //
    return res
  }

  data() {
    // Next in the sequence. And will fire the 'data' event: this.emit('data', data)
  }

  error(err) {
    this.emit('error', err)
  }
}

After that I have the data method to be called. My doubt is: Is there a design pattern to call the events in sequence without using Promises? Currently I'm using chaining, but I'm feeling that this isn't the best approach, maybe I'm wrong.

this.initialize()
  .then(this.downloadFile)
  .then(this.data)
  .catch(this.error)

But I feel that could be a better approach.


Answers for bergi's questions:

a) Why are you using class syntax?

Because it's easier to inherit from EventEmitter and personally I think it's more readable than using a constructor functin, e.g:

function Transformation(data) {
  this.data = data
}

// Prototype stuffs here

b) How this code is going to be used

I'm creating a client to interact with my API. The ideia is that the user can see what is happening in the background. E.g:

const data = {
 data: {},
 format: 'xls',
 saveTo: 'path/to/save/xls/file.xls'
}

const transformation = new Transformation(data)

// Events
transformation.on('initialize', () => {
  // Here the user knows that the transformation already started
})

transformation.on('fileDownloaded', () => {
  // Here the file has been downloaded to disk
})

transformation.on('data', (data) => {
  // Here the user can see details of the transformation -
  //   name,
  //   id,
  //   size,
  //   the original object,
  //   etc
})

transformation.on('error', () => {
  // Here is self explanatory, if something bad happens, this event will be fired
})

c) What it is supposed to do?

The user will be able to transform a object with data into a Excel.

FXux
  • 415
  • 6
  • 15
  • 1
    You [definitely should not put the promise chain in the constructor](https://stackoverflow.com/q/24398699/1048572) (and it's totally unclear why this is supposed to be a `class`) – Bergi Feb 14 '18 at 16:55
  • No, promises are *the* solution for running things in sequence (you might of course substitute your chaining with `async`/`await`, you're already using it anyway). It's much more cumbersome with events, why are you trying to? – Bergi Feb 14 '18 at 16:56
  • You sent a answer saying that is a bad thing to return a Promise from the constructor, but I'm not @Bergi – FXux Feb 14 '18 at 17:40
  • True, you're not returning it, but that's even worse :-D. My answer is the linked thread explains why it is a bad thing to start asynchronous actions in constructors in general. – Bergi Feb 14 '18 at 17:53
  • Yes, I saw, I'm going to change the code to avoid that. @Bergi, don't you think is weird to see a implementation that uses Promises, but emits events? – FXux Feb 14 '18 at 17:55
  • Yes, I think this whole implementation is weird :-) However, to suggest an alternative you'd need to tell me a) why you used `class` syntax b) how this code is going to be used c) what it is supposed to do d) whether the events are used for anything else but sequencing the actions. – Bergi Feb 14 '18 at 17:59
  • Extra observation: I'm using the EventEmitter to interact with the user, but in the class I use Promises because each method is dependent of the previous one. This happens because in my API i'm creating and updating the record. – FXux Feb 14 '18 at 18:25
  • I would really suggest using the factory pattern here. It's fine to use `class`, but I would expect a class instance to be fully operational whenever I instantiate it. Forcing me to call `initialize` smells. Have the factory construct the instance, kick off the initialization, and return a Promise that will resolve with the concrete instance after initialization is complete. – zero298 Feb 14 '18 at 18:28
  • Consider the API that [`Observable.webSocket()`](http://reactivex.io/rxjs/class/es6/Observable.js~Observable.html#static-method-webSocket) has. You create something that will do async things and emit values over time. You start the whole process through subscription. – zero298 Feb 14 '18 at 18:32
  • I can't, I'm using the Serverless framework, unfortunately websockts don't work. – FXux Feb 14 '18 at 18:37
  • Do you mind to write an example of the factory implementation? @zero298 – FXux Feb 14 '18 at 18:37

2 Answers2

1

It sounds like the transformation object you are creating is used by the caller solely for listening to the events. The user does not need a class instance with properties to get or methods to call. So don't make one. KISS (keep it super simple).

function transform(data) {
  const out = new EventEmitter();
  async function run() {
    try {
      const url = await initialise();
      const data = await downloadFile(url);
      out.emit('data', data);
    } catch(err) {
      out.emit('error', err);
    }
  }

  async function initialise() {
    const req = superagent.post('url...')
    superagent.send(data)
    const res = await req // this will actually fire the request
    out.emit('initialize')
    return res.body
  }

  async function downloadFile(url) {
    const req = superagent.put(url)
    req.on('progress', (progress) => out.emit('downloadFile', progress)
    const res = await req; // this will actually fire the request
    //
    // save to disk
    //
    return data;
  }

  run();
  return out;
}

It might be even simpler to leave out the (once-only?) data and error events and just to return a promise, alongside the event emitter for progress notification:

  return {
    promise: run(), // basically just `initialise().then(downloadFile)`
    events: out
  };
BananaNeil
  • 10,322
  • 7
  • 46
  • 66
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Genius! I didn't know that I don't need a class to use the EventEmitter. Thank you Bergi! <3 – FXux Feb 14 '18 at 19:29
0

If you want another way to call the events in sequence, and if you're using a Node.js version that supports ES7, you can do the following :

class MyClass extends EventEmitter {
  constructor(data) {
    this.data = data;

    this.launcher();
  }

  async launcher() {
    try {
      await this.initialize();
      await this.downloadFile();
      await this.data();
    }
    catch(err) {
      this.error(err);
    }
  }

  initialize() {
    const req = superagent.post('url...');
    superagent.send(data);
    this.emit('initialize');
    this.url = req.body;
    return req;
  }

  downloadFile() {
    const req = superagent.put(this.url);
    req.on('progress', (progress) => this.emit('downloadFile', progress)
    //
    // save to disk
    //
    return req;
  }

  data() {
    // Next in the sequence. And will fire the 'data' event: this.emit('data', data)
  }

  error(err) {
    this.emit('error', err)
  }
}

Explanation : instead of awaiting for your Promises inside your functions, just return the Promises and await for them at root level.

Guerric P
  • 30,447
  • 6
  • 48
  • 86
  • I think `initialize` will get error res is not defined. – HMR Feb 14 '18 at 15:34
  • yeah, edited, I forgot this when rewriting the code. There is another problem to deal with by the way : load the response body on request response, otherwise it might remain empty – Guerric P Feb 14 '18 at 16:22