open-wc / custom-elements-manifest

Custom Elements Manifest is a file format that describes custom elements in your project.
https://custom-elements-manifest.open-wc.org/
225 stars 37 forks source link

[Analyzer]: Custom Events declared outside of dispatchEvent are not registered #191

Open Matsuuu opened 1 year ago

Matsuuu commented 1 year ago

Checklist

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

Expected behavior

Heya. Was testing out the event analysis on the playgrounds and noticed that events are not registered unless they are instantiated inside the dispatchEvent -method.

Repro:

https://custom-elements-manifest.netlify.app/?source=CmNsYXNzIE15RWxlbWVudCBleHRlbmRzIEhUTUxFbGVtZW50IHsKICBzdGF0aWMgZ2V0IG9ic2VydmVkQXR0cmlidXRlcygpIHsKICAgIHJldHVybiBbJ2Rpc2FibGVkJ107CiAgfQoKICBzZXQgZGlzYWJsZWQodmFsKSB7CiAgICB0aGlzLl9fZGlzYWJsZWQgPSB2YWw7CiAgfQogIGdldCBkaXNhYmxlZCgpIHsKICAgIHJldHVybiB0aGlzLl9fZGlzYWJsZWQ7CiAgfQoKICBmaXJlKCkgewogICAgdGhpcy5kaXNwYXRjaEV2ZW50KG5ldyBFdmVudCgnZGlzYWJsZWQtY2hhbmdlZCcpKTsKICB9CgogIGZpcmVzVG9vKCkgewogICAgdGhpcy5kaXNwYXRjaEV2ZW50KG5ldyBDdXN0b21FdmVudCgnbXktY2hhbmdlZC1ldmVudCcpKTsKICB9CgogIGRvZXNOb3RGaXJlKCkgewogICAgICBjb25zdCBteUV2ID0gbmV3IEV2ZW50KCJteS1ldmVudCIpOwogICAgICB0aGlzLmRpc3BhdGNoRXZlbnQobXlFdik7CiAgfQoKICBkb2VzTm90RmlyZUVpdGhlcigpIHsKICAgICAgY29uc3QgbXlFdiA9IG5ldyBDdXN0b21FdmVudCgibXktb3RoZXItZXZlbnQiKTsKICAgICAgdGhpcy5kaXNwYXRjaEV2ZW50KG15RXYpOwogIH0KfQoKY3VzdG9tRWxlbWVudHMuZGVmaW5lKCdteS1lbGVtZW50JywgTXlFbGVtZW50KTsK&library=vanilla

As you can see, the events object only has the events declared inside the method call, and not outside of it.

Code


  fire() {
    this.dispatchEvent(new Event('disabled-changed'));
  }

  firesToo() {
    this.dispatchEvent(new CustomEvent('my-changed-event'));
  }

  doesNotFire() {
      const myEv = new Event("my-event");
      this.dispatchEvent(myEv);
  }

  doesNotFireEither() {
      const myEv = new CustomEvent("my-other-event");
      this.dispatchEvent(myEv);
  }

Output

 "events": [
            {
              "name": "disabled-changed",
              "type": {
                "text": "Event"
              }
            },
            {
              "name": "my-changed-event",
              "type": {
                "text": "CustomEvent"
              }
            }
          ]
Matsuuu commented 1 year ago

Looking at the analyzer code (https://github.com/open-wc/custom-elements-manifest/blob/master/packages/analyzer/src/features/analyse-phase/creators/createClass.js#L159-L197)

We can see that the only arguments it's parsin are the NewExpression inside the dispatchEvent node.

I'd be happy to look into making this parsing better if there's ask for it. I'll be building something similiar to the go-to-definition of CELS (Custom Elements Language Server) on events so this would go hand in hand with that.

thepassle commented 1 year ago

Sure, happy to review a pr for this

jamieomaguire commented 1 year ago

Hey folks 👋 I also noticed this issue today and was wondering if there was ever a PR to address it? @Matsuuu

Matsuuu commented 1 year ago

@jamieomaguire heya! Had some off-github discussions with Passle about this and came to the conclusion that tracking this info requires parts of TS Lang client that would exponentially multiply the load of analysis when activated.

So for now, at least, we've shelved the idea. Could maybe take a look at this further down the line but it would require quite a lot of rework on the whole tool to implement

jamieomaguire commented 1 year ago

Ah that's tricky, makes sense though. Thanks for letting me know!

ullmark commented 2 months ago

Hi, the issue with not supporting this is also that I haven't been able to do an event that can be prevented and also documented correctly. ex these scenarios:

// this will be correctly parsed
fireEvent1() {
  /** Event description here */
  this.dispatchEvent(new CustomEvent("an-event", { bubbles: true, composed: true, cancelable: true }));
}

// Here it will find the event but the description aren't found
fireEvent2() {
  /** Event description here */
  if (!this.dispatchEvent(new CustomEvent("an-event", { bubbles: true, composed: true, cancelable: true }))) {
  }
}

// Here it will find the event but the description aren't found
fireEvent3() {
  /** Event description here */
  const notPrevented = this.dispatchEvent(new CustomEvent("an-event", { bubbles: true, composed: true, cancelable: true }));
  if (notPrevented) {

  }
}

// Here it won't find the event at all.
fireEvent4() {
  const event = new CustomEvent("an-event", { bubbles: true, composed: true, cancelable: true });
  /** Event description here */
  this.dispatchEvent(event);
  if (event.defaultPrevented) {
  }
}