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

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

Open
1 of 2 tasks
Matsuuu opened this issue Dec 8, 2022 · 6 comments
Open
1 of 2 tasks

Comments

@Matsuuu
Copy link
Contributor

Matsuuu commented Dec 8, 2022

Checklist

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

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
Copy link
Contributor Author

Matsuuu commented Dec 8, 2022

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
Copy link
Member

Sure, happy to review a pr for this

@jamieomaguire
Copy link

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

@Matsuuu
Copy link
Contributor Author

Matsuuu commented Jun 30, 2023

@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
Copy link

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

@ullmark
Copy link

ullmark commented May 2, 2024

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) {
  }
}

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

No branches or pull requests

4 participants