Add typescript version of event aggregator

[ctoran]

Generates more precise type declarations including:

• overloads for pub/sub methods
• type inference of the message using generics
• hides private class members

Build it with gulp build

This PR replaces the original ES6 event aggregator with the equivalent TypeScript code. The TS code is first compiled to ES6 using typescript and then to ES5 using babel for each module type. Also SourceMap is generated in outputs so you can step-through the source TS code in debugger while running an ES5 compilation.

To make it easier to review the change, just diff index.ts with the old index.js (see here).

[EisenbergEffect]

@ctoran Is there some reason you couldn't add these types into our source?

[ctoran]

Yes, there is a reason. It is not possible to specify these typings in Babel with overloads and generics. I did this as an experiment to compare the quality of type definitions emitted from a strongly-typed TS code and what we get from typings that are added to Babel/JS code. With the .d.ts definitions outputted from Babel, you can still use the API in a wrong way. Whereas the .d.ts definitions from TS compiler fully document the usage.

[EisenbergEffect]

Hmmm. I really don't want to maintain two versions of the source. Perhaps we can just commit the d.ts only. I'll need to think about it.

[ctoran]

If you commit the .d.ts. only, you will still have to maintain two things, and manually keep the definition file in sync with the source. I've had success with two step compilation first from ts to ES6 and then from ES6 to ES5 using Babel. With this configuration all ES modules load correctly, you maintain a single source, and you can use some of the TypeScript compiler options that can be enabled only when ES6 is targetted such as async functions. The ES6 output from TS compiler exactly matches what you currently have in the repository. Another interesting thing with this configuration is that SourceMap works flawlessly, so you can debug the code in the browser and step through the original TS code (or step through the babel code if you remove the secondary source map). This is what I would suggest.

[ctoran]

I have a working sample of two-step compilation for app-contacts here:
https://github.com/ctoran/app-contacts/tree/master-typescript

[ctoran]

Hi @EisenbergEffect, I know your team is busy with the upcoming release, but here this is for whenever you had time to review.

I updated the typescript event-aggregator with latest changes (subscribe methods returning subscription object) and modified the build to do two-step compilation from TS to ES6 and then from ES6 to each of the supported ES5 modules. The generated ES6 is very similar to the original js code and two level source maps allow debugging it in either ES6 or TS.

The main benefit that comes from this is the more precise definitions that are auto-generated without having to maintain them separately. To compare, here is the original definition outputted from babel:

declare module 'aurelia-event-aggregator' {
  import * as LogManager from 'aurelia-logging';
  export interface Subscription {
    dispose(): void;
  }
  class Handler {
    constructor(messageType: any, callback: any);
    handle(message: any): any;
  }
  export class EventAggregator {
    constructor();
    publish(event: string | any, data?: any): void;
    subscribe(event: string | Function, callback: Function): Subscription;
    subscribeOnce(event: string | Function, callback: Function): Subscription;
  }
  export function includeEventsIn(obj: Object): EventAggregator;
  export function configure(config: Object): void;
}

And here's the new .d.ts file generated by TypeScript:

export interface Subscription {
    dispose(): void;
}
export declare class EventAggregator {
    private eventLookup;
    private messageHandlers;
    publish<T>(event: T): void;
    publish(event: string, data?: any): void;
    subscribe<T>(event: Constructor<T>, callback: (message: T) => void): Subscription;
    subscribe(event: string, callback: (message: any, event?: string) => void): Subscription;
    subscribeOnce<T>(event: Constructor<T>, callback: (message: T) => void): Subscription;
    subscribeOnce(event: string, callback: (message: any, event?: string) => void): Subscription;
}
export declare function includeEventsIn(obj: any): EventAggregator;
export declare function configure(config: any): void;

As we discussed before, it is not possible to add some of the signatures such as overloads and generics to babel code. The new declarations from TS fully document the usage of the library and provide better intelisense while the old one allows the consumer code to use the API in a wrong way in many ways: access private classes or members, pass function parameters with wrong signature, pass an object instance to Subscribe methods instead of class name, use wrong object type for message, etc.

I think this kind two-step compilation gives us the best of both worlds and avoids need to maintain a separate or community version of the definitions.

[EisenbergEffect]

@ctoran Thanks for doing this work! Let me take this back to my team and let us discuss how we think we should approach this.

[EisenbergEffect]

Just dropping a quick note to let you know I haven't forgotten this. I'm pondering it all....

[EisenbergEffect]

@ctoran For the moment, we want to keep our primary source in ES2016. There are a number of reasons for this, both political and technical. I'd rather not get into it all here. I really appreciate all the work you have done on this. Would you be willing to submit a PR with the d.ts file only? For now what I think we want to do is use your d.ts file and not try to generate our own for this library. This library isn't likely to change much if at all for a long time, so the maintenance costs involved in having to keep the d.ts file updated would be little to none. So, would you be willing to submit a PR that just fixes up the build to not generate the d.ts and instead uses the hand-coded version you have?

[EisenbergEffect]

@ctoran On second thought....what if for this repo we ditched Babel altogether. Would you be willing to set it up so that it was 100% TypeScript? We are investigating the possibility of moving Aurelia in that direction, especially since the very low quality and unprofessional release of Babel 6. I think this library and your use cases might be a good start to see what the process would be like.

Interested?

[EisenbergEffect]

@ctoran Any interest?

[atsu85]

@ctoran, @EisenbergEffect

@ctoran On second thought....what if for this repo we ditched Babel altogether. Would you be willing to set it up so that it was 100% TypeScript? We are investigating the possibility of moving Aurelia in that direction, especially since the very low quality and unprofessional release of Babel 6.

That is exactly something I've hoped to hear from @EisenbergEffect for very long time. I hope @ctoran will make it happen.

[andrevlins]

if @ctoran can't do it for any reason I bet @cmichaelgraham or other typescript master can.

[atsu85]

I guess it is worth to mention another issue where (amont other things) i proposed using TypeScript. @EisenbergEffect took some time to point out the 9 concerns with adopting TypeScript.and it seems to me, that none of them will become a problem - please review his concerns and my response: aurelia/http-client#111 (comment)
(and correct me if i'm wrong)

[Lakerfield]

@EisenbergEffect See commit: Lakerfield@9303f99 for a buildscript where the Babel compiler is replaced with the TypeScript compiler. The only thing whats needed to build is a copy of aurelia-logging.d.ts in the typings directory.

[EisenbergEffect]

Does this also include working d.ts generation for the library and successful karma test runs and api.json generation?

[Lakerfield]

The d.ts generation is working. Currently testing the karma test runs. How can i generate a api.json?

[EisenbergEffect]

You can run gulp doc to generate the api.json file for our docs. I have a feeling that that task will need to be fixed up to work after your changes though...

[EisenbergEffect]

@Lakerfield I downloaded your fork, ran npm install and jspm install. However, the build fails due to it not being able to find aurelia-logging. Is there an additional step to get it working? This is actually one of the major concerns I've had about moving to TS...cross-repo d.ts dependencies.

[Lakerfield]

@EisenbergEffect currently you have to copy the aurelia-logging.d.ts manually to the typings folders. Its possible to script the copy in the build or if not found download from github or http://definitelytyped.org/tsd/

There is a new commit with a fix in the build.js for the gulp doc command

The karma test run is almost working, currently having a problem with jspm framework and typescript preprocessor plugins together

[atsu85]

Just mentioning for clarity, that this pull request may have created some confusion - this pull request is created by @ctoran , but @Lakerfield also mentioned his work, that is now taken over the discussion ;)

Regarding that

[atsu85]

@EisenbergEffect :

However, the build fails due to it not being able to find aurelia-logging.

@Lakerfield :

you have to copy the aurelia-logging.d.ts manually to the typings folders

I don't think this is the best solution. It seems to me, that @ctoran solved it better - he's build-ts uses tsconfig.json (see https://github.com/aurelia/event-aggregator/pull/13/files#diff-40e62be8220b9aeab02f5a664980bd73R12) that also includes jspm dependencies for TS compiler, so build can find it from jspm_packages folder - no manual copy needed. (Just as a note to @Lakerfield, if manual copy should be needed, then probably it should be scripted in package.json with "scripts" element, but i think it is not needed)

[EisenbergEffect]

Yes, we would want to use the d.ts files that are deployed with our source code through jspm. We would need the build set up to take those into account for sure.

[Lakerfield]

@EisenbergEffect, @atsu85 The typescript build script is rebuild and now uses the *.d.ts from aurelia source code through jspm Lakerfield@e1161d3 (ts-build branch)

gulp build and gulp doc are working. karma start is working on my machine, but needs some cleanup before I can commit.

[atsu85]

@Lakerfield , i see lots of replacements like that:

- ea.eventLookup
+ (<any>ea).eventLookup

in Lakerfield@c9772ea

Why are You casting ea to any instead of EventAggregator that seems to be the correct type? https://github.com/Lakerfield/event-aggregator/blob/master/src/index.ts#L29
I havent tested if it works, I'm missing smth?

@ctoran , can You also join this discussion (it's Your pull request after all that @Lakerfield has "hijacked" ;) ). Maybe You can collaborate on this effort?

[Lakerfield]

@atsu85 The fields are defined as "private" (not really but typescript validates it)
The unittest is checking inside the class its private fields, and uses any to skip the typescript validation.

It was also possible to make the fields public, then no cast was needed, but then the fields where visible inside the d.ts files.

[atsu85]

The fields are defined as "private" (not really but typescript validates it)

quite obvious - i must be almost sleeping.

[atsu85]

@Lakerfield , i now took time to review how You solved it:

The typescript build script is rebuild and now uses the *.d.ts from aurelia source code through jspm Lakerfield/event-aggregator@e1161d3 (ts-build branch)

I'm really not happy with that approach, i added the comment there as well:
Lakerfield@e1161d3#diff-51890d29cd8fe41e2c98b079f9cd3fcaR10

but it is basically what i said before:

It seems to me, that @ctoran solved it better - he's build-ts uses tsconfig.json (see https://github.com/aurelia/event-aggregator/pull/13/files#diff-40e62be8220b9aeab02f5a664980bd73R12) that also includes jspm dependencies for TS compiler

The positive effect of using tsconfig.json instead your approach is that all the ides and even command line tsc would build the same way (with the same include path, target and other compiler flags - you can overwrite module format in gulp for different package formats you want to create with gulp build).

[ctoran]

Hi guys, sorry I was away for a while and missed the discussion. @EisenbergEffect please feel free to use the .d.ts file, no need to wait for a PL from me. I mostly did this as a proof of concept to see how it works. Regarding ditching Babel for TypeScript I had some issues with loading modules when targeting TS directly to ES5. Also some experimental features like async/await work only when ES6 is targeted, so even though using 100% TS may work for this project, it won't work very well for larger projects. That's why I ended up using dual compilation. Sorry again for not being around to answer.

[EisenbergEffect]

FYI We aren't going to put this in to the Beta because that is only a few days away and I don't want to make a big, risky change this close to the deadline. I'm keeping this open because I think we should continue to consider this post Beta.

[atsu85]

FYI We aren't going to put this in to the Beta because that is only a few days away...

makes sense.

@EisenbergEffect so by "this" You meant @ctoran's version (this pull request), not @Lakerfield's version, that also gained a lot attention in this pull request discussion? I'd expect @ctoran and @Lakerfield to review each-others changes and come up with the best solution they can collectively think of - probably both solutions could be improved, so as tempting as it seems to start using TS ASAP, it makes sense not to rush with it before beta.

[EisenbergEffect]

By "this" I mean the general idea of converting this repo to TS. Right now we are heads down getting everything ready for the Beta and I don't want to make any large changes before then. After that, we can look at unifying the work of @ctoran and @Lakerfield into a single PR that gets everything set up for a "best of breed" build process. Depending on how that goes, we will probably look at converting other repos, one-by-one in the same way.

[ctoran]

I just resolved the conflicts. My Event Aggregator index.ts and @Lakerfield's are almost identical. I think the difference is mostly in the build flow (Quick link for comparing). Also I did not modify test files. I still prefer two-step TS/Babel builds as it plays better with ES6 modules and async functions.

[EisenbergEffect]

FYI One of the reasons we might move to TS, would be to eliminate all dependency on Babel.

[atsu85]

@ctoran considering current situation with Babel6, it really makes sense to ditch Babel, as Rob suggested - at least for now, for this repository, to get fully working proof-of-concept out to get things moving.

So my suggestion would be:

• compile with TS only (as @Lakerfield did)
• use tsconfig.json file (as @ctoran did, so You don't have to duplicate compiler flags for gulp and all IDEs and even tsc command line tool)
• use tsconfig.json to include third-party d.ts files from jspm_packages (as @ctoran did)
• update tests (as @Lakerfield did)
• make sure, that doc is properly generated (as @Lakerfield did)

@Lakerfield, @ctoran what do You think? Would You be willing the other party to use Your code to create the pull request?

[ctoran]

@atsu85

@Lakerfield, @ctoran what do You think? Would You be willing the other party to use Your code to create the pull request?

Yes.

Also looking at TypeScript roadmap it looks like the limitations I had with modules and async functions will go away by TS 2.0, so eventually compiling with only TypeScript compiler would be enough. Babel from the beginning was a temporary solution for me.

[atsu85]

it looks like the limitations I had with modules and async functions will go away by TS 2.0

Yes, TS team has stated, when talking about TS 2.0 goals, that

The first is to align with ES6. Aligning with ES6 allows TypeScript to become a superset of the next JavaScript

[glen-84]

You should probably add "compileOnSave": false to the tsconfig.json file. It's not a standard option yet, but at least it helps with Atom.

See microsoft/TypeScript#2326

[atsu85]

@glen-84

You should probably add "compileOnSave": false to the tsconfig.json file

Why?
I've used Atom for TS for half year, and haven't felt the need to disable Atom compiling. Based on all my experiments, both gulp and Atom produce 100% same results (including formatting, whitespaces, line endings and so on). Maybe You need to tweak some compiler options, mine are following:

    "compilerOptions": {
        "target": "es5",
        "noEmitOnError": true,
        "module": "amd",
        "declaration": false,
        "emitDecoratorMetadata": true,
        "experimentalDecorators": true,
        "newLine": "LF",
        "noImplicitAny": false,
        "removeComments": false,
        "noLib": true
    },

[glen-84]

@atsu85 Because if you're watching & recompiling using Gulp, you don't want the files to be compiled more than once.

[EisenbergEffect]

I'm going to close this for now. If anyone is interested, I think we will need a clean PR with everything set up. It would need to replicate all our existing functionality, gulp build, unit testing, linting, etc.