Hagsten / Talkify

Javascript Text to speech library
216 stars 32 forks source link

Don't chain the `.play()` method. #40

Closed BobStein closed 5 years ago

BobStein commented 5 years ago

One of the drawbacks to fluent API is you can get confused about return values. This confusion shows up in the very first "working fiddle" at http://jsfiddle.net/98gdLvph/

It has this misleading code:

var playlist = new talkify.playlist()
  .begin()
  .usingPlayer(player)
  .withRootSelector('#root')
  .withTextInteraction()
  .build()
  .play();

This assigns undefined to playlist, because .play() has no return value. Instead I suggest:

var playlist = new talkify.playlist()
  .begin()
  .usingPlayer(player)
  .withRootSelector('#root')
  .withTextInteraction()
  .build();
playlist.play();

Similarly misleading is the code in README.md at https://github.com/Hagsten/Talkify#user-content-play-all-top-to-bottom

new talkify.playlist()
    .begin()
    .usingPlayer(player)
    .withTextInteraction()
    .withElements(document.querySelectorAll('p')) //<--Any element you'd like. Leave blank to let Talkify make a good guess
    .build() //<-- Returns an instance.
    .play();

I think it should be more like this:

var playlist = new talkify.playlist()
    .begin()
    .usingPlayer(player)
    .withTextInteraction()
    .withElements(document.querySelectorAll('p')) //<--Any element you'd like. Leave blank to let Talkify make a good guess
    .build();
playlist.play();
Hagsten commented 5 years ago

Hi, thank you for your input.

It is unfortunate that the examples are misleading. In the readme file there is stated that "build" returns the instance and it is only for simplicity that play() is chained.

I might adjust the examples when I get the time :)

Hagsten commented 5 years ago

I've changed all the examples, hopefully it will be more clear now. Closing issue.