Bug: incorrect event names when using custom event classes

[WickyNilliams]

Checklist

• [x] Did you run the analyzer with the --dev flag to get more information?
• [x] Did you create a minimal reproduction in the playground?

Completing the items above will greatly improve triaging time of your issue.

Expected behavior

I have included links to playground at various steps.

Given some code like this:

this.dispatchEvent(new MyEvent(someArg))

CEM genearates output like:

"events": [
  {
    "name": "someArg",
    "type": {
      "text": "MyEvent"
    },
  }
],

Which is the incorrect - it uses the event argument identifier for the event name. I do not expect CEM to get the actual name from the MyEvent class, but rather not try to infer the name in this case.

If instead I try to give CEM a hint for the event name like this:

/** @fires my-event - a description for my event */
this.dispatchEvent(new MyEvent(someArg))

The output is the same as above.

If I instead add the @fires tag to the class:

/** 
 * @fires my-event - a description for my event
 */
class MyElement extends HTMLElement {
  fire() {
    const someArg = { hello: "world" }

    this.dispatchEvent(new MyEvent(someArg));
  }
}

Then the output contains two events instead of one:

"events": [
  {
    "name": "someArg",
    "type": {
      "text": "MyEvent"
    }
  },
  {
    "description": "a description for my event",
    "name": "my-event"
  }
],

Some discussion on slack arrived at the following workaround, separating out the event construction from dispatchEvent, combined with the @fires tag on the class:

/** 
 * @fires my-event - a description for my event
 */
class MyElement extends HTMLElement {
    fire() {
        const someArg = { hello: "world" }
        const event = new MyEvent(someArg)
        this.dispatchEvent(event);
    }
}

which now has the correct output:

"events": [
  {
    "description": "a description for my event",
    "name": "my-event"
  }
],

But it is unfortunate that you have to structure the code in this way. I assume this is a bug, some combination of these should be true:

• CEM does not use an argument's identifier as event name
• the inline @fires tag at the dispatchEvent call site should do the right thing (the docs imply this should work)
• the @fires tag on class with inline event construction should only output one event

As a (related) aside, the docs also show this pattern:

/** @type {FooEvent} foo-event - description */
this.dispatchEvent(new FooEvent('foo-changed'));

But bennyp seems to think this is incorrect and should not be used like this. Perhaps it should simply @fires instead of @type?

[thepassle]

We should improve the handling of events here: https://github.com/open-wc/custom-elements-manifest/blob/master/packages/analyzer/src/features/analyse-phase/creators/createClass.js#L171

We currently only expect new Event('whatever') there, not really custom event classes. We maybe should only do that if its a new Event or new CustomEvent and the first argument is a string literal. (e.g.: not a new WhateverEventName)

Also it looks like the jsdoc handling here https://github.com/open-wc/custom-elements-manifest/blob/master/packages/analyzer/src/features/analyse-phase/creators/createClass.js#L185 doesnt work as intended, i'd expect that to overwrite the event, so we should also improve on that.

I'd be happy to take a PR for this

[WickyNilliams]

We maybe should only do that if its a new Event or new CustomEvent and the first argument is a string literal.

Yeah I think it should only be for Event and CustomEvent. For any other user-defined event, you cannot be sure a string arg is the event name e.g. new SomeEvent("some-string-that-is-not-the-event-name")

I'll see if i can make a PR for this during the week. Thanks for the pointers!