ml-in-barcelona / server-reason-react

Server render Reason React components with OCaml natively
https://ml-in-barcelona.github.io/server-reason-react/local/server-reason-react/index.html
MIT License
132 stars 8 forks source link

Set event callbacks running to be browser_only by default. #162

Open pedrobslisboa opened 2 months ago

pedrobslisboa commented 2 months ago

The idea is to create a feature that converts all event callbacks into browser_only, as events can only be called on a client.

Recently, I created a PR to work on this idea, but I made it a bad idea. Check it out here: https://github.com/ml-in-barcelona/server-reason-react/pull/159#:~:text=Why%20this%20PR%20was%20closed

I'm opening this issue to discuss how and if it is worth having this feature.

davesnx commented 2 months ago

The idea of adding browser_only to all props that start with on is probably worth exploring. Do you want to start by re-making the PR and trying to add to all 'onClick' props first?

pedrobslisboa commented 2 months ago

@davesnx I was thinking about this: The idea of adding browser_only to all props that start with on is probably worth exploring

The on prefix is commonly used for HTML event callback, but the user cannot use it like this. So, working on this could provide some issues, don't it?

davesnx commented 2 months ago

Probably, but users that want to name something differently props like (handleWhatever) they would need to add browser_only manually, in case of adding onWhatever it will be added automatically.

jchavarri commented 2 months ago

I was thinking about this: The idea of adding browser_only to all props that start with on is probably worth exploring

How will this solution avoid the issue with unused variable warnings mentioned in #159?

jchavarri commented 2 months ago

To my taste, adding browser_only to all props starting with on* is too much magic happening behind the scenes. I would probably start adding browser_only just on the list of known on* props for lower case components, test that solution first, and see how it goes.