sonus icon indicating copy to clipboard operation
sonus copied to clipboard

Convert index.js to TypeScript

Open pbalaram opened this issue 8 years ago • 8 comments

With these changes, we can now use ES7 features.

class CloudSpeechRecognizer {
    constructor() {
        // code here
    }
}

I figure this might help keep things organized, as the codebase grows.

Bonus: You can now run examples/example.js by running the following.

npm run example

pbalaram avatar Feb 26 '17 08:02 pbalaram

Thanks for the PR! This looks great, but I think we don't need classes. I'd rather move to using typescript and interfaces. That way we can get all the es7+ features and avoid classes

ashishsc avatar Feb 26 '17 20:02 ashishsc

Sure, TypeScript makes sense. I've converted the core library to use it. I'm not sure why we would avoid classes, would you mind explaining a bit further?

pbalaram avatar Feb 27 '17 02:02 pbalaram

Frankly I could go either way on this, but @ashishsc has strong feeling about code style that I typically defer to. Regardless, the one change I would request is that you include a prepublish script in package.json using

npm run build

That way I never accidentally publish the npm package without building the library first :)

evancohen avatar Feb 28 '17 03:02 evancohen

Thanks for converting to typescript! we <3 you.

As far as I why I don't prefer classes. I like being able to pass the sonus object in as an easy way to mock the methods. Also this will be a breaking change.

However, I come from a functional programming background (internal state makes things harder to test, not that we have any) but @evancohen has more object-oriented experience so since he's fine with classes, by all means we should go with that.

ashishsc avatar Feb 28 '17 03:02 ashishsc

Thank you for the feedback!

@evancohen I've added a prepublish step, which should call npm run build. @ashishsc I've added tslint, fixed the trivial warnings, and added rules for the more widespread ones.

pbalaram avatar Mar 26 '17 21:03 pbalaram

I think with the linter setting enabled we can already avoid these issues unless I'm mistaken?

On Sun, Mar 26, 2017, 7:30 PM Prithvi Balaram [email protected] wrote:

@pbalaram commented on this pull request.

In src/sonus.ts https://github.com/evancohen/sonus/pull/35#discussion_r108082426:

  •  threshold: 0,
    
  •  device: this._device || null,
    
  •  recordProgram: this._recordProgram || "rec",
    
  •  verbose: false
    
  • });
  • this._mic.pipe(this._detector);
  • this._started = true;
  • }
  • public stop() {
  • stopRecording();
  • }
  • public pause() {
  • this._mic.pause();

Correct! They did resolve this particular issue, my intent was simply to suggest that we might be able to avoid this class of issues entirely by keeping semi-colons mandatory.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/evancohen/sonus/pull/35#discussion_r108082426, or mute the thread https://github.com/notifications/unsubscribe-auth/ACgRCtNWQm3pci1KjIvo9dH5rATC-GLsks5rpx80gaJpZM4MMSst .

ashishsc avatar Mar 27 '17 04:03 ashishsc

Hey folks! I want to get this merged, but things were left in a bit of an ambiguous state. @ashishsc are we good on the linter settings?

evancohen avatar Apr 27 '17 20:04 evancohen

I'm good with what we have currently, we can always change it later. We should just use an autoformatter and reduce bikeshedding at some point. Once pretter finishes their TS support, I'll add that in here.

ashishsc avatar May 04 '17 04:05 ashishsc