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

Run checkEncoder with template haskell #1087

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

Conversation

alexfmpe
Copy link
Contributor

@alexfmpe alexfmpe commented Jun 15, 2024

Got the idea of using TH for encoders long ago from @ryantrinkle.

This has the side effect of speeding up reloads by doing the check only when Common.Route is modified instead of every reload. Likely not to be noticed unless relying on large Universe instances.
EDIT: Hmm no it doesn't. It's pretty unsatisfying that the lack of a Lift instance (at least when using -> and Kleisli m categories) means we need to check twice. All we get is upgrading fails-at-startup to fails-at-compile-time.

I marked this as draft because everytime Common.Route.Checked is recompiled in ghci (and thus ob commands) I start getting

<interactive>: ^^ Could not load 'CommonziRouteziChecked_validFullEncoder_closure', dependency unresolved. See top entry above.


ByteCodeLink.lookupCE
During interactive linking, GHCi couldn't find the following symbol:
  CommonziRouteziChecked_validFullEncoder_closure
This may be due to you not asking GHCi to load extra object files,
archives or DLLs needed by your current session.  Restart GHCi, specifying
the missing library using the -L/path/to/object/dir and -lmissinglibname
flags, or simply by naming the relevant files on the GHCi command line.
Alternatively, this link failure might indicate a bug in GHCi.
If you suspect the latter, please report this as a GHC bug:
  https://www.haskell.org/ghc/reportabug


...done

until I recompile Backend.hs again. Not using validFullEncoder in Backend.hs also stops it from triggering

I have:

  • Based work on latest develop branch
  • Followed the contribution guide
  • Looked for lint in my changes with hlint . (lint found code you did not write can be left alone)
  • Run the test suite: $(nix-build -A selftest --no-out-link)
  • Updated the changelog
  • (Optional) Run CI tests locally: nix-build release.nix -A build.x86_64-linux --no-out-link (or x86_64-darwin on macOS)


backend :: Backend BackendRoute FrontendRoute
backend = Backend
{ _backend_run = \serve -> serve $ const $ return ()
, _backend_routeEncoder = fullRouteEncoder
, _backend_routeEncoder = hoistCheck (pure . runIdentity) validFullEncoder
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could just be _backend_routeEncoder = validFullEncoder but that's a breaking change, probably best to get adoption first (assuming we merge this)

Comment on lines +14 to +20
case fullRouteEncoder of
Left err -> error $ Text.unpack err
Right _ ->
[d|
validFullEncoder :: Encoder Identity Identity (R (FullRoute BackendRoute FrontendRoute)) PageName
Right validFullEncoder = fullRouteEncoder
|]
Copy link
Contributor Author

@alexfmpe alexfmpe Jun 15, 2024

Choose a reason for hiding this comment

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

Can't do this directly in Common.Route due to stage restriction.
I would also like to have some sort of thisIsTheOnlyBindingYouShouldEdit = fullRouteEncoder before the case so that renames to fullRouteEncoder in "userland" only require a single rename in this weird place (and thus unlikely to get out of sync) but that requires yet another module due to stage restriction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't able to pull this into a DRY util as there's no way to get a Lift instance for Encoder to splice back in.
At most we could have a util taking in Encoder check parse a b for the check and Name/Dec for use in the new binding and pass both "forms" of fullRouteEncoder. Maybe it'd be worth putting that in Obelisk.Route ?

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.

1 participant