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

Attempt at supporting shorter syntax #2

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

benadamstyles
Copy link
Contributor

Fixes #1

This is just an exploration, to test the limitations of what could be achieved. The problem with the JSX syntax is that using JSX calls ReasonReact.element, which means bs-epitath doesn't get the opportunity to pass in the render prop function, hence why currently the user must declare a function.

In this attempt, I assumed 2 limitations:

  1. The user shouldn't need to use a function declaration in the let%Epitath binding (i.e. we are aiming at a shorter syntax).
  2. We can only use the let-anything ppx, we musn't require a further ppx.

The only solution I could think of was this PR, where the user simply passes the component's make function and it's up to bs-epitath to call ReasonReact.element with that make function and also pass in the render prop function. (NB I switched round the meaning of children in this PR because it didn't make sense to me that children referred to the render prop component and render referred to the children function!).

This is the diff of usage:

// before
let%Epitath {pageX, pageY} = children => <OnLayout> ...children </OnLayout>;

// after this PR
let%Epitath {pageX, pageY} = OnLayout.make;

There are 2 issues with this solution, however:

  1. It doesn't use JSX syntax, which means it's not as intuitive. As I say above, I can't see a way around this.
  2. It doesn't currently offer a way to pass props to the render prop component other than the children prop. There might be a way around this!

Refactor to avoid the user needing to pass a function
@benadamstyles
Copy link
Contributor Author

Ah! 🤦‍♂️ I've just worked out that problem 2 above is nonexistent! This already works:

// before
let%Epitath {pageX, pageY} = children => <OnLayout callback=Js.log> ...children </OnLayout>;

// after this PR
let%Epitath {pageX, pageY} = OnLayout.make(~callback=Js.log);

So the only downside remaining is the lack of JSX syntax.

@fakenickels
Copy link
Contributor

amazing @benadamstyles , sorry the delay to comment.
About 2 I was about to say about it indeed your conclusion.
Looks a nice concession to kill verbosity this for me.
Not thinking in any corner case that would be problematic with this implementation

@benadamstyles
Copy link
Contributor Author

Cool 🙂 the only thing is the inability to pass ref or key, but I don’t think I’ve ever seen that with a render prop component?

@fakenickels
Copy link
Contributor

Me neither @benadamstyles

@hew
Copy link

hew commented Feb 14, 2019

I like this.

@fakenickels
Copy link
Contributor

Well, so it's a go!

@fakenickels fakenickels merged commit b34242c into Astrocoders:master Feb 14, 2019
@fakenickels
Copy link
Contributor

Thanks for your contribution @benadamstyles, will release soon to npm

@fakenickels
Copy link
Contributor

@benadamstyles we are thinking in keep both syntaxes and call this one other let%Zombie just to keep in the vibe of Epitath haha

@benadamstyles
Copy link
Contributor Author

Hey, yep seems like a good idea to keep both syntaxes, for those who prefer JSX visually.

Does zombie have a meaning in programming already? Thinking of “zombie processes”. Just wondering about choosing a term with fewer existing connotations!

@benadamstyles
Copy link
Contributor Author

What about just let%Epi (because it’s shorter, like this new syntax)

@guilhermedecampo
Copy link
Member

guilhermedecampo commented Feb 21, 2019

Makes sense also! we could even go furthe let%E

@benadamstyles
Copy link
Contributor Author

Yep! Although as module names are global, maybe that’s a little risky?

@fakenickels
Copy link
Contributor

Epi looks good!

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.

None yet

4 participants