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

feat: generalize ResponseM to any MonadAff #16

Merged
merged 7 commits into from
Sep 13, 2024

Conversation

cakekindel
Copy link
Contributor

No description provided.

@f-f
Copy link
Collaborator

f-f commented Oct 3, 2023

@cakekindel what's the usecase for this?

@cakekindel
Copy link
Contributor Author

I noticed that Response (and maybe some other structures) are generic over any MonadAff and am running HTTPurple in a custom pipe-based MTL, and figured this would improve the UX a bit.

See CustomStack for the concrete value-add.

I removed ResponseM because it's not super useful anymore but this change does not have to be breaking since it can easily maintain the previous API while just adding the general server' function I wrote.

Note that there has also been an open issue in httpure for several years if you want a little more context: #134

@sigma-andex
Copy link
Owner

thx @cakekindel , currently on vacation. Will have a look at it next week.

@EmpiricEmpire
Copy link

Will have a look at it next week.

Which one, is it the week on September 16 or the one on October 7-th?

Copy link
Collaborator

@f-f f-f left a comment

Choose a reason for hiding this comment

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

I think this is a good change - CI is currently failing and the logs are not available anymore, @cakekindel could you have a look at this?

@cakekindel
Copy link
Contributor Author

cakekindel commented Sep 12, 2024 via email

@cakekindel
Copy link
Contributor Author

@f-f can you approve the CI run?

@cakekindel
Copy link
Contributor Author

I don't know how the tests ever passed - 2 main issues:

  1. mockResponse didn't implement Stream#write correctly, so the tests that tried to write to those objects always timed out. Replaced the record with a MockResponse class extending http.OutgoingMessage
  2. all tests that ran serve had race conditions where requests were issued before the server was actually running. For some, I used a serveAwaitReady function that resolves an Aff with {onStarted :: Maybe (Effect Unit)}. For others that already specified that, I just did the naiive thing of spinning until a request can connect

@cakekindel
Copy link
Contributor Author

just noticed my git & gpg emails mismatched, force push was just changing the committer email

@sigma-andex sigma-andex merged commit 0534903 into sigma-andex:main Sep 13, 2024
1 check passed
@sigma-andex
Copy link
Owner

@cakekindel awesome! thanks!

@f-f I don't know what the current process is to publish a new version with the new registry.

@f-f
Copy link
Collaborator

f-f commented Sep 13, 2024

@sigma-andex if you cut a new tag it should be picked up by the registry, but switching out the spago.dhall with a spago.yaml should be good for future proofing

@f-f
Copy link
Collaborator

f-f commented Sep 13, 2024

The registry currently has a cronjob to pick up all legacy packages once a day, but using spago publish will publish it immediately

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.

4 participants