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

EventEmitter events #17

Closed
rogierschouten opened this issue Jun 25, 2014 · 11 comments
Closed

EventEmitter events #17

rogierschouten opened this issue Jun 25, 2014 · 11 comments
Labels
enhancement Improved functionality

Comments

@rogierschouten
Copy link

Hi Sebastian,

You really filled a void with this tool, thank you! I'm currently deriving a class from EventEmitter, and I was searching for a way to document the events being emitted. Would it be an idea to introduce an extra @event tag? I'd be happy to try and implement this if you could give me some pointers as to how to do so.

Rogier

@sebastian-lenz
Copy link
Member

Hi Rogier,

great to hear that the project is of use for you. Actually I'm also in the need of a way to document events as TypeDoc is in large parts event-based. I've included @event tags within my doc comments, although they are not working yet:

/**
 * Triggered once per project before the dispatcher invokes the compiler.
 * @event
 */
static EVENT_BEGIN:string = 'begin';

However, there are several drawbacks to this approach. Defining events this way is not very javascript-ish, I know only very few libraries that handle event definitions this way. Also static variables are not inherited but an event declaration should also apply for child classes. Finally one cannot document event parameters this was.

Maybe we should move the event documentation to the class level and include it within the class doc comment:

/**
 * @event begin  Triggered once per project before the dispatcher invokes the compiler.
 * @param e Event  The first event parameter.
 */
class EventEmittingClass extends EventEmitter {}

I would extend TypeDoc.Models.Comment to include the event models. Comments are parsed in TypeDoc.Factories.CommentHandler so the mapping would happen right there. TypeDoc invokes CommentHandler for each inherited class and the handler opts out in it's onDeclaration method, to enable inheritance some changes are needed here.

Finally we must render the events within the partials/member.hbs template, maybe we should also include them within the navigation at TypeDoc.Output.DefaultTheme.getNavigation() and within the member index at partials/index.hbs.

If you would like to contribute to this project you are highly welcome.

Sebastian

@rogierschouten
Copy link
Author

Ok I'll start working on it.

How do you envision multiple event comments? One comment block or multiple?

I initially thought of a third option, how about this?

/**
 * Some description of the class
 */
class EventEmittingClass extends EventEmitter {

  /**
   * @event begin  Triggered once before the dispatcher invokes the compiler.
   * @param e {Event}  The first event parameter.
   */

  /**
   * @event end Triggered once after the dispatcher invokes the compiler.
   * @param e {boolean} The first event parameter.
   */

}

This has the advantage that IF you use these static string declarations, or some other way of putting it in a string, you can still do so and put them near the event comments. Also I slightly prefer this style because the comment blocks can be separated per event while still obviously belonging to the class.

@poelstra
Copy link
Contributor

This sounds like a very good idea indeed!
I agree with Rogier that having separate comment blocks makes the individual events more readable, and may provide better separation while parsing (e.g. in case unknown @-tags are parsed, to which event do they belong).
Also, putting curl-braces around the type makes it more Javadoc compliant.

However, I'm not quite sure what to think of these 'free-standing' blocks. It feels to me like they should be followed by some kind of (non-comment) declaration, like Sebastian's proposal.

I was even wondering whether literally specifying string-based e.g. overloads of "on" might be possible somehow, although we will have to provide a 'real' implementation if we do so...

I was trying the following in the Playground:

class EventEmitter {
    on(event: string, listener: Function): EventEmitter {
        return this;
    }
}

class Something extends EventEmitter {
    // This one seems to be necessary before the specialized overloads
    on(event: string, listener: Function): Something;
    /**
     * @event Emitted at start of something.
     * But how to describe 'n' here?
     */
    on(event: "begin", listener: (n: number) => void): Something;
    /**
     * @event Emitted at end of something.
     */
    on(event: "end", listener: (x: string) => void): Something;
    on(event: string, listener: Function): Something {
        super.on(event, listener);
        return this;
    }
}

var s = new Something();
function f(n: number): void {
    console.log(n);
}
s.on("begin", f);
s.on("end", f); // <== this should yield compile error, but it doesn't
// Intellisense does work though, it shows the call signatures of
// the expected handlers for begin and end!

@sebastian-lenz
Copy link
Member

Cool, that not only looks more typescript-ish, it actually brings in additional typings that could be understood by the compiler. Event the TypeScript specification shows this usage as an example for string constant types. This way of documenting events for sure is the first class choice when writing declaration files. But within an actual project I would like to avoid to have to rewrite the actual function body of the "on" method every time.

Everything done by the documentation generator till know is tightly bounds to the way the TypeScript compiler works. As events are not part of the language itself (for obvious reasons) this is the first feature introduced by this project itself. It would be great to find a solution that covers multiple use cases of the @event tag as all code examples on this page appear reasonable to me.

As you point out in you example, "sub"-parameters cannot be documented right now, I'm working on this problem right now.

@rogierschouten: Having separated comment blocks per event definition indeed looks more clearly laid out. I'm not sure how this is reflected by the declarations returned by the compiler. The TypeScript.AST instance attached to declarations returns an array of comments through preComments()/postComments(), I guess that those comment blocks are attached to the next valid declaration.

@rogierschouten
Copy link
Author

I understand now why you say the document generator is tightly bound to TypeScript ;) . This is a way bigger change than I thought it would be. If we want to make events first-class citizens, it seems we'll have to redefine things like the TypeScript.PullElementKind enum. That will cause extra maintenance with next typescript versions, as it will no longer stay in sync. Are you sure this is what you want?

@sebastian-lenz
Copy link
Member

I would prefer not to not modify the TypeScript.PullElementKind and TypeScript.PullElementFlags enumerations. Thought about that before and quickly ran into trouble. In order to be able to easily update TypeScript without breaking any functionality, we should keep using their implementations.

Just a guess, are you trying to implement events as DeclarationReflection instances? We will run into trouble here as models are merged with declarations having the same name, so an event "click" and a method "click" will end up as one reflection. Also the reflection cannot be processed by the Dispatcher as there is no corresponding PullDeclaration.
Instead of adding the kind "event" to the TypeScript.PullElementKind enumeration, it would make more sense to flag certain models with an additional "isEvent" member. The kind could still be a variable or a function.

What about attaching the events to the Comment model? Did you dismiss it for a certain reason?

@jesperstarkar
Copy link

What is the syntax you ended up with here?

@aciccarello
Copy link
Collaborator

I think this was implemented as just marking symbols as an event type. The test example is just a static string property.

class EventDispatcher
{
    /**
     * This is an event documentation.
     * @event
     */
    static EVENT_CLICK:string = 'click';

}

lddubeau pushed a commit to lddubeau/typedoc that referenced this issue Apr 5, 2018
@cancerberoSgx
Copy link

I've written some alternatives to describe/document events in TypeDoc here : https://cancerberosgx.github.io/javascript-documentation-examples/examples/typedoc-tutorial-basic/docs/docco/src/index.html#events

Personally I don't like the static properties approach, it force me to declare extra properties, and it doesn't allow me to declare events at the interface level. Also it has the same problems at the proposed by @rogierschouten (event names appear stacked under a method name) and it makes it hard to see and
maintain the relationship between event name and corresponding listener signature.

In that document I analyze this two proposals and I propose a third one that IMO present events optimally, Basically is declaring the listener signature type outside the interface and name it like the event. (so it encapsulate the relationship event name-listener signature). Tag it with @event and in the overrides of method on() emit() etc use it to reference the listener signature. Then the hack is instructing the plugin to move the type as a @event member of the interface. Well, conceptually I don't know if it is a hack or not since the concept of events doesn't exist in JavaScript/TypeScript. The plugin is called "as-member-of" and jsdoc3 has some similar tags like @member or @Lends so not sure if it's too bad.

If you could review that material and give me your opinion I would very appreciate it. Hope it could help at least as a starting point for a discussion :)

Thanks.

@rogierschouten
Copy link
Author

@cancerberoSgx I agree that method 3 is best but why is there a need to use @event at all? Given that typings are being standardized with these addListener signatures, TypeDoc can maybe figure out on its own that these are events.

@cancerberoSgx
Copy link

So we agree that what .d.ts authors are doing overloading addListener(), on(), etc for strict handler typechecking is the way to go for declaring / documenting events, right ?

Do you think TypeDoc should do "magic" and treat EventEmitter methods in a special way to automatically generate event members ? Do you think that users shouldn't use @event but the tool automatically create these event members?

If so, I like the idea, as long as this magic is done by a plugin and not typedoc core. Will try to prototype something when I have time. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improved functionality
Projects
None yet
Development

No branches or pull requests

6 participants