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

Less strict checking of event handler attribute names #164

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

pedro00dk
Copy link
Contributor

@pedro00dk pedro00dk commented Nov 12, 2024

Hi!

I'm having some issues with solid eslint plugin regarding asynchronous custom event handlers.

const comp = <my-element on-click={async () => {}} />
// warns:
// This tracked scope should not be async. Solid's reactivity only tracks synchronously. (eslintsolid/reactivity)

Just the line above is enough to reproduce it in playground. I checked other ways to describe event handlers and noticed full lowercase handlers have the same issue.

const comp = <div
    on-click={async () => {}} // WARNS HERE
    on:click={async () => {}}
    onclick={async () => {}} // WARNS HERE
    onClick={async () => {}} 
/>

After investigating the output, I also found this has nothing to do with event delegation as onclick is delegated. Custom event handlers are also properly compiled and event listeners are created for them, meaning they are not expected to mutate.

Therefore, this should be a linter issue. After going through the repo, I found that there is a distinction between event handlers ("called-function" scope) and other ordinary functions ("function" scope).

The reactivity error is emited here because the on-click is not considered a "called-function".
https://github.com/solidjs-community/eslint-plugin-solid/blob/main/packages/eslint-plugin-solid/src/rules/reactivity.ts#L818

A bit down I found the issue was this check:
https://github.com/solidjs-community/eslint-plugin-solid/blob/main/packages/eslint-plugin-solid/src/rules/reactivity.ts#L853
The regex is too strict for my case, causing the if condition to fail and fall into an else if a bit down:
https://github.com/solidjs-community/eslint-plugin-solid/blob/main/packages/eslint-plugin-solid/src/rules/reactivity.ts#L914
That causes the scope is set as "function" instead of "called-function".

The solution was just about changing the condition that decides the scope.
Initially, I though about just changing the regex to support my case: /^on[-:A-Z]/ (note extra hyphen).
But I remembered onclick is also not covered: /^on[-:A-Za-z]/
(lowercase event handlers are already covered by another rule, so I thought this would be fine.)

Then, I decided to look into https://github.com/ryansolid/dom-expressions to see what checks are used there to define what is considered an event handler.
https://github.com/ryansolid/dom-expressions/blob/main/packages/dom-expressions/src/client.js#L349

It turns out all checked is if the property starts with "on" or not. Hence, I decided to do the same thing and just check if the property starts with "on" as well.

Copy link
Collaborator

@joshwilsonvu joshwilsonvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me! Nice investigation and thanks for the PR.

@joshwilsonvu joshwilsonvu merged commit 70937dc into solidjs-community:main Nov 12, 2024
3 checks passed
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

Successfully merging this pull request may close these issues.

2 participants