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

Allow pathTo and parse to work with Maybe (Id record) #1691

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

amitaibu
Copy link
Collaborator

@amitaibu amitaibu commented Jun 2, 2023

fixes #1671

  • Fix render link
  • Fix parse route
  • Tests

@amitaibu amitaibu marked this pull request as draft June 2, 2023 18:50
@amitaibu
Copy link
Collaborator Author

amitaibu commented Jun 2, 2023

@mpscholten I've got the tests red, which is good - as I haven't fixed the route parsing yet 😅 . Can you please point me in the right direction for the parsing part

@amitaibu
Copy link
Collaborator Author

amitaibu commented Jun 2, 2023

I suspect it's here. Looks scary! 😨

@amitaibu
Copy link
Collaborator Author

amitaibu commented Jun 2, 2023

So if I understand the code correctly, if we wanted to return a Just [here] we'd have to do (

Just uuid -> uuid |> unsafeCoerce |> Right
)

- Just uuid -> uuid |> unsafeCoerce |> Right
+ Just uuid -> Just uuid |> unsafeCoerce |> Right

But we can't know when it's a Maybe and when a regular. So one option would be keeping the Just in the URL, and parse that. So this URL will be valid NewThing?redirect=Just03142938-62d5-4de1-af90-49726ca98f35 (note the Just in the query param)

Nothing ->
-- We couldn't parse the UUID, so try Maybe (Id record),
-- where we have a @Just@ prefix before the UUID.
queryValue
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mpscholten Is there a way to debug such a function and have it print the value of queryValue ?

Copy link
Collaborator Author

@amitaibu amitaibu Jun 2, 2023

Choose a reason for hiding this comment

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

Well, I've just fixed it "Just" not "Just ". But still would be nice to know if there's a way to debug

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mpscholten It's indeed a tricky problem. Is there a way to debug/print the values in those functions?

Copy link
Member

Choose a reason for hiding this comment

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

@amitaibu
Copy link
Collaborator Author

amitaibu commented Jun 2, 2023

After fiddling with the parser, I think it would be easier to keep the Just prefix and Nothing in the URL.

Just Nothing
http://localhost:8000/NewThing?redirect=Justbbc7604b-09bd-4ce5-a0e4-92409d1de4d9 http://localhost:8000/NewThing?redirect=Nothing

Tests are passing now 😄

@amitaibu amitaibu marked this pull request as ready for review June 2, 2023 19:56
@amitaibu amitaibu changed the title Remove Just prefix from query params Allow pathTo and parse to work with Maybe (Id record) Jun 2, 2023
@amitaibu amitaibu requested a review from mpscholten June 3, 2023 12:25
@mpscholten
Copy link
Member

This implementation is inconsistent with how other Maybe types are working with AutoRoute. E.g. when I have a field myField :: Maybe Int, and I request /MyAction?myField= it will set myField = Nothing, and /MyAction?myField=1337 means Just 1337.

This part of IHP is a bit tricky as you've seen already :D But there's likely a way we get this consistent with the other Maybe implementations

@amitaibu
Copy link
Collaborator Author

amitaibu commented Jun 3, 2023

I was looking at the code some more, but I don't know how to proceed.

Maybe I'm completely off, but I remember reading somewhere that in the past there was Generic that was added to data which was removed due to long comiple time. Could that be useful in our case, by adding it to to the data FooController ? Maybe with the GHC v9 we're using it would be quicker?

In short, I'm happy I was able to tackle it up to this point, but will need help/ someone to take it from here ✌️

@mpscholten
Copy link
Member

Yeah, this is a tricky problem. I've already tried it myself yesterday and couldn't find a quick solution so far. Will take a deeper look soon

@amitaibu
Copy link
Collaborator Author

amitaibu commented Oct 22, 2023

I just hit this one when implementing a Chat (with ihp openAI) on my app. Tried:

AiChatsAction { maybeAiChatId :: !(Maybe (Id AiChat)) }

Since it's not currently working, as a workaround I can change this to :

AiChatsAction { AiChatId :: !(Id AiChat) } and pass an empty UUID 0000-0000-... (doing that with (newRecord @AiChat).id). But it's not great.

@amitaibu
Copy link
Collaborator Author

as a workaround I can change this to :

Actually, I don't need a workaround (or this feature). I can simply go with

= AiChatsAction
| NewAiChatAction

So I guess this feature is still needed from time to time, but likely there are workarounds.

@mpscholten
Copy link
Member

I run into this problem as well every once in a while. Hope we can fix this at some point 👍 I usually follow the same workaround

@CSchank
Copy link
Contributor

CSchank commented Nov 20, 2023

Just ran into this - I ended up converting everything in actions to be Maybe Text and then I converted back to UUIDs from there in the actions. A part of my Haskell soul died in doing so. But it works as expected, in terms of not including the parameters if they're Nothing. I wonder, if it can be done for Text, why can't it be done for UUIDs and other things? Shouldn't it work in the same way for all Maybe types, regardless of the type parameter?

@mpscholten
Copy link
Member

Shouldn't it work in the same way for all Maybe types, regardless of the type parameter?

Yes. The current implementation of AutoRoute doesn't allow for it. It likely needs an internal refactoring to fix this issue

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.

links to Actions with Maybe's get literal Just in query params
3 participants