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

Templating support #2

Closed
wants to merge 4 commits into from
Closed

Conversation

complyue
Copy link
Collaborator

@complyue complyue commented Jun 4, 2024

I realized Koka 3.1.2 has been released a few days ago, so here is the cleaner PR aiming for merge, obsoleting #1

Please review this PR and advise on the typing error, also you'd like it get merged?

with val page-title = "The Front Page"

with fun body-content() // admit no content from the template, fresh new here
h3
Copy link
Member

Choose a reason for hiding this comment

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

The root of the issue stems from here.
The problem is that h3 requires the html-tree effect but create-component : (() -> <html-tmpl|e> component) -> e component has e as the underlying effect. This causes a unification with html-tree which is not handled above this handler. (Remember that handler functions are run after yielding to the context of their handler).

Bidirectional effects (which Koka doesn't have, but we could add a feature request for), as far as I understand them allow you to state that the effect function should be called in the context of the call to the effect, and not first yield up to the handler. In this way some effects from the call site can be used. (And html-tree would be handled in this case).

My first attempt at solving the typing problem here, was to change the handlers to use val body-context = fn() ... which was sort of a workaround to try to do bidirectional effects in Koka. The problem with this approach was with the polymorphic effect. We can continue trying this approach if that is what you wanted.

The final solution in #3 (sorry for not pushing the code up earlier), changes the design a bit so that the body-content and other functions return a component rather than using the effect directly. I've added another version of the component API which handles the html-tree without overriding the effect allowing you to build up a subpiece of the tree. Which was probably a good addition anyways. There are a few things about that API that might not have been as clean as your current attempt, but at least it works and type checks. Anyways, I would love to get your feedback there if those API changes seem acceptable. (And it looks like you added to the example an external effect here which I'll have to migrate over).

Copy link
Member

@TimWhiting TimWhiting left a comment

Choose a reason for hiding this comment

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

This is a commit with the changes needed to get just the layout.kk file to typecheck using your approach. As you can see there is an html-tree due to your default handlers using src/script which needs an html-tree handler above it - but you handle all html-tree effects below it (at the html root).

@complyue
Copy link
Collaborator Author

complyue commented Jun 4, 2024

Thanks @TimWhiting !

I'd love to explore alternative approaches too, enjoying the diving into effect tracking at type level ;-)

But first I need to make one approach working at least, currently I'm stuck with this error:

examples/front.kk(50, 3): type error: effects do not match
  context        :   with create-component <- layout-html(tmpl)
                   ...
                     with
  term           :   with
  inferred effect: <html/layout/body-begin-scripts,html/layout/body-end-scripts,html/layout/body-end-scripts,html-builder/html-tree|_e>
  expected effect: <html/layout/html-tmpl,html-builder/html-tree>

Failed to compile examples/front.kk

why _e there not inferred to be an empty effect row?


btw, maybe you can suggest some more concise way for the temple to provide "default" effects? I feel the create-component callback as in current approach somehow cumbersome...

@TimWhiting
Copy link
Member

Sorry if I was not clear, the approach in #3 works, so if you want to start from something that works you can take a look at that.

@TimWhiting
Copy link
Member

I agree that I don't really like the create-component callback. I'll give it some thought later tonight.

@complyue
Copy link
Collaborator Author

complyue commented Jun 4, 2024

Sorry I didn't realize that #3 is anew, thinking that's still #1 which I've closed. Looking into #3 definitely ...

@complyue
Copy link
Collaborator Author

complyue commented Jun 5, 2024

#3 is a perfect fix along the direction of this approach, closing this in preference of #3

@complyue complyue closed this Jun 5, 2024
@TimWhiting TimWhiting mentioned this pull request Jun 5, 2024
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