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

Feature: Add option to dispatch events instead of using global functions #203

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shahata
Copy link

@shahata shahata commented Oct 4, 2024

No description provided.

@shahata shahata changed the title Add option to dispatch events instead of using global functions Feature: Add option to dispatch events instead of using global functions Oct 4, 2024
@christopherjbaker
Copy link
Contributor

christopherjbaker commented Oct 8, 2024

We had been planning to add something like this for a while, so thank you ! I like the general idea here, though I'm concerned about a couple of things:

  • this fully changes the behaviour of all function props
  • the final WC is expected to not have a matching attribute
  • erroneous getters/setters are added to the prototype.

I have 2 possible approaches, with no strong preference between them. Whichever you think would be better:

  1. Introduce an events object on the options. The keys would be in the React prop format as they are now, and the values would be like the value of the dispatchEvents object you added. This would resolve all 3 above, and I think the only needed change would be to loop over the events object after the propNames loop that's there. It would still modify this[propsSymbol][prop] as it does now.
  2. Introduced an "event" type instead of using "function". This would leave the code structure mostly as you have it now, though the condition you introduced would just check for type === "event" and it should log a warning in dev mode if they provide the value in an attribute, and it should skip over these props when adding the descriptors lower in the funtion.

Does that make sense?

@shahata
Copy link
Author

shahata commented Oct 8, 2024

Sounds great, I will update the pr soon

@christopherjbaker
Copy link
Contributor

christopherjbaker commented Oct 8, 2024

After talking this over more with Brad, we'd like to stick with my first suggestion above. The second one limits the ability to customize individual event options, and we have some ideas for future expansions that would also need this.

If you are having any trouble, don't fully understand the suggestion, or just don't have time, I'd be happy to offer my assistance to help with or finish the PR. You've already done a huge part of it.

@shahata
Copy link
Author

shahata commented Oct 8, 2024

@christopherjbaker I adjusted the implementation according to your first suggestion. I'm not sure this is 100% what you meant, so plz let me know before I update the docs.

@shahata
Copy link
Author

shahata commented Oct 23, 2024

@christopherjbaker any chance you can take a look?

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