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

Partial query evaluation #576

Open
wants to merge 6 commits into
base: master
from

Conversation

4 participants
@garyb
Copy link
Member

garyb commented Sep 23, 2018

Check it out @natefaubion!

Two-fold rationale:

  1. we looked into it before of course, but I couldn't figure out the plumbing - so the same reasons as then, allows proxy components to fail properly, etc.
  2. I wanted to make a less boilerplatey construction method for the components again, and by allowing query eval to return Nothing that leaves us free to have the query type polymorphic until a real eval is provided for it.

I've only updated two of the examples, for proof that it's usable, as I wanted to see if we can think of reasons why we shouldn't do this.

@garyb garyb requested a review from natefaubion Sep 23, 2018

, handleQuery :: forall a. f a -> HalogenM s act ps o m (Maybe a)
, receive :: i -> Maybe act
, initialize :: Maybe act
, finalize :: Maybe act

This comment has been minimized.

@garyb

garyb Sep 23, 2018

Member

I guess these could be Maybe (HalogenM s act ps o m Unit) rather than going via action handling, but I kinda like handling these in one place (which is already true if HalogenQ is handled rather than going the mkEval route).

This comment has been minimized.

@natefaubion

natefaubion Sep 23, 2018

Collaborator

I don't think they would need to be Maybe, since they can just be pure unit.

This comment has been minimized.

@garyb

garyb Sep 23, 2018

Member

That's true - it was more a comment on them mapping to act rather than operating directly in HalogenM.

@natefaubion
Copy link
Collaborator

natefaubion left a comment

I think this looks good. Since we have Request, maybe Handle should be Action?

, finalize :: Maybe act
}

mkEval

This comment has been minimized.

@safareli

safareli Sep 25, 2018

Contributor

What you think about calling mkEval from mkComponent (i.e. ComponentSpec taking EvalSpec ?

This comment has been minimized.

@garyb

garyb Sep 25, 2018

Member

We can't do that without exposing another different version of mkComponent, since Halogen uses it internally. Plus I wanted to provide the choice of writing an eval in terms of HalogenQ directly.

Having mkEval in there seemed better given both of those considerations, as it should avoid some confusion about things like when we had 4 different component constructors before - they were all the same aside from some synonyms and field-defaulting, but that wasn't obvious to the casual user. Since the same actual component constructor is used here, and we just use another function to build up part of its internals, it shows that there's nothing special going on, no matter how the component is constructed.

@acple

This comment has been minimized.

Copy link
Contributor

acple commented Sep 29, 2018

more I like mkEval has this type:

mkEval
  :: forall s f act ps i o m
   . (EvalSpec s f act ps i o m -> EvalSpec s f act ps i o m)
  -> HalogenQ f act i ~> HalogenM s act ps o m

does no longer require expose defaultEval.
eval: H.mkEval _{ handleAction = handleAction }

@garyb

This comment has been minimized.

Copy link
Member

garyb commented Sep 29, 2018

I did consider that too, or exporting a function that does that as well as having a defaultEval record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment