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

Change prerender return to expose initial value #447

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

alexfmpe
Copy link
Member

As is, this is a breaking change. Migration strategy is pretty much to add a uncurry holdDyn in front of prerender.

A non-breaking alternative is to add a separate function, say, prerender', though that makes more work for the implementation:

  • we can supply a default method implementation prerender a b = uncurry holdDyn =<< prerender' a b, but that would add the MonadHold constraint to all the implementations that don't supply their own prerender, which might be its own breaking change. For instance, these two instances would now require extra constraints on the function that tries to use them
    • instance (SupportsStaticDomBuilder t m) => Prerender t (StaticDomBuilderT t m) where
    • instance (Prerender t m, Monad m) => Prerender t (ReaderT r m) where
  • we can also factor out the common code to avoid extra constraints
    instance (ReflexHost t, Adjustable t m, PrerenderBaseConstraints t m) => Prerender t (HydrationDomBuilderT GhcjsDomSpace t m) where
      type Client (HydrationDomBuilderT GhcjsDomSpace t m) = HydrationDomBuilderT GhcjsDomSpace t m
      prerender a b = pure <$> commonImpl a b
      prerender' a b = (commonImpl a b, never)
    
    commonImpl :: m a -> Client m a -> m a
    commonImpl _ client = client

but the lack of let bindings in classes makes this very awkward.

I'm not sure the pain to make this non-breaking is worth it especially given we'd then end with 3 very similar functions:

  • prerender
  • prerender_
  • prerender'

Copy link
Member

@ryantrinkle ryantrinkle left a comment

Choose a reason for hiding this comment

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

Great change; we should make it less-breaking though. E.g. maybe add a new function to the class so we only break instances instead of breaking all use sites.

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