Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Base64 get/setCookie and documentation to Obelisk.Frontend.Cookie #1017

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

Conversation

parenthetical
Copy link

@parenthetical parenthetical commented Jan 25, 2023

This extends the Cookie module with a class for setting cookies and adds some documentation to the module.

HasSetCookie is implemented on Snap and MonadJSM/Client m. The Haddocks warn that HasSetCookie and HasCookies do not necessarily implement a state monad. While this is the case for Client m, in Snap setting cookies affects the HTTP response while asking cookies looks at the request. I did this for pragmatic reasons (not breaking old code, not requiring a wrapper type inside Snap to distinguish the two sources of cookies).

A few helper functions are added to encourage Base64 use by default: setCookie and askCookie work do Base64 encoding/decoding of cookies.

  • 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) Failed on some unrelated points?
  • 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)

Closes #977.

@madeline-os madeline-os self-requested a review January 26, 2023 15:05
Copy link
Collaborator

@madeline-os madeline-os left a comment

Choose a reason for hiding this comment

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

Looks good. Just tiny requests

@@ -10,26 +10,72 @@
{-# LANGUAGE RankNTypes #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE UndecidableInstances #-}
{-# LANGUAGE OverloadedStrings #-}
{-|
Description:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Documentation 💜




-- TODO: Make generic over both frontend and backend.
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo in code >:|

can you make an issue instead?

askCookies = fmap (parseCookies . encodeUtf8) $ DOM.getCookie =<< askDocument

-- | Store a cookie which will be Base64 encoded.
setCookie :: (HasSetCookie m) => SetCookie -> m ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we name this encodeAndSetCookie? I don't want it to be surprising that this function re-encodes your cookie value for you.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants