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

add renderSumType to efficiently render sum types #115

Closed
wants to merge 1 commit into from

Conversation

anderspapitto
Copy link

I believe this is complete enough that we can figure out if/where it has a home upstream in reflex-dom.

See inline comment on renderSumType.

This function provides an alternative to two approaches to sum types
that already exist

  • using dyn/widgetHold. This approach has the downside that the Dynamic
    is lost while rendering everything under the sum type, which means
    that every change causes a full re-render.

  • rendering every constructor, and having a dynamic attribute with
    'display:none' set for everything except the currently active
    constructor. This works but is less principled - you are rendering
    something that doesn't actually exist, and then hiding it, instead of
    simply not rendering it in the first place (the approach of
    renderSumType).

See inline comment on renderSumType.

This function provides an alternative to two approaches to sum types
that already exist

- using dyn/widgetHold. This approach has the downside that the Dynamic
  is lost while rendering everything under the sum type, which means
  that every change causes a full re-render.

- rendering every constructor, and having a dynamic attribute with
  'display:none' set for everything except the currently active
  constructor. This works but is less principled - you are rendering
  something that doesn't actually exist, and then hiding it, instead of
  simply not rendering it in the first place (the approach of
  renderSumType).
@3noch
Copy link
Member

3noch commented May 22, 2017

Can this be merged?

@adamConnerSax
Copy link

This has overlaps with reflex-frp/reflex#106.

  1. It also uses generics-sop so the additional dependency overlaps although this is for reflex-dom and that is for reflex.
  2. It also has a mapping from a sum-type which is an instance of Generic to a DSum over the functor (NP I). But the tag here is different from what I did in the reflex work and, while these tags must be isomorphic, I think, for consistency, one should be settled on before either bit of work is merged.
    I need to understand how this tag works before knowing if I can redo my work in these terms but at first glance it looks easier to work with.
    However we do it, the Generic t -> DSum pieces should live someplace in reflex so that both new bits can use the same code.

@anderspapitto
Copy link
Author

anderspapitto commented May 24, 2017

My memory is a little fuzzy by now, but here are a few points

Can this be merged?

It is complete and it does what I wanted it to do. From that perspective it can be merged (or someone, e.g. @adamConnerSax can cannibalize bits of it into other PRs).
The reasons against merging would be

  • there's no current "champion" of this code. I wrote it, but I'm not currently using reflex/reflex-dom and don't have further time to put it. If it's going to be included, probably someone else who wants this functionality and understands the code should take it over.
  • it hasn't been well tested. It's short, typed code that gets past the compiler, but... probably it should still have some tests written before merging.
  • from discussion with @ryantrinkle, it could potentially be generalized further in some ways which I don't quite remember. It's unclear to me whether it's best to hold off on that.

But the tag here is different from what I did in the reflex work and, while these tags must be isomorphic, I think

From a brief look at your PR, it looks like my toDSum is a little easier to use, because it converts all the way from the original Generic t => t to the final DSum. It's also a lot shorter, so it's probably missing functionality compared to what you have. Also note, the main goal I had was to hide as much of the generic-programming complexity from the end-user of Reflex-Dom as possibe. If you look at the example in the in-line comment (near https://github.com/reflex-frp/reflex-dom/pull/115/files#diff-cbc18a1ad0bb45d24e2369d7e6230321R95), that's code that the user has to write. It's very simple and formulaic though, even for someone who doesn't understand what an NP I is.

tldr; I'm equally fine with this being merged, with it being closed as no-intention-to-merge, or with someone else taking over to see it through.

@adamConnerSax
Copy link

The tags are isomorphic, I just needed a good nights sleep to see why. You cleverly re-used NS as the choice element of the tag type (more or less), whereas I created a new type to represent a choice of elements of a type level list.
I don't think your tag or toDSum is missing any functionality.
The PRs aim for different things and, concretely, overlap only in the use of mapping sum-types to DSum via generics-sop.
But I do think Ryan (hello, @ryantrinkle !) has a whole set of things he thinks are useful to do with these sorts of manipulations and perhaps that should be the guide to how to move forward here.

Adam

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.

3 participants