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

Register json middleware #1331

Merged
merged 1 commit into from
Oct 8, 2021
Merged

Register json middleware #1331

merged 1 commit into from
Oct 8, 2021

Conversation

mollerhoj
Copy link
Contributor

I was quite surprised to realize I had to do register this middleware manually, as I didn't have to do that for the other middleware in core.

@iMacTia
Copy link
Member

iMacTia commented Oct 8, 2021

OMG @mollerhoj thank you for pointing this out, I'm surprised we didn't catch this before!
You made me realise I missed a few things when I moved the json middleware from faraday_middleware.
Your PR addresses one of these things but not the others, let me see if I can add them myself so we get this done in one batch

@iMacTia
Copy link
Member

iMacTia commented Oct 8, 2021

@mollerhoj I'm unable to commit 😢, could you see if the "allow edits by maintainers" checkbox is enabled on the right?

@mollerhoj
Copy link
Contributor Author

It is :-)

Screen Shot 2021-10-08 at 11 06 27

Now that I have your attention: I'm reading through this gem, and I can't seem to understand what the point of this registry and the adapter registry is. Why not just add the middlewares classes directly to the connection with something like:

Faraday.new do |f|
  f.request AuthorizationMiddleware
end

The registry seems to add needless complexity (and apparently race conditions #1065). Is there a good reason for it I just do see?

@iMacTia
Copy link
Member

iMacTia commented Oct 8, 2021

For some reason I can't commit on your branch in any way 🤔

Screenshot 2021-10-08 at 11 35 14 am

Could you cherry-pick or replicate this commit into your PR?

I'm reading through this gem, and I can't seem to understand what the point of this registry and the adapter registry is. Why not just add the middleware classes directly

This is mostly for legacy support, and you correctly pointed out is not necessary at all.
You can simply add the middleware class, though in that case you'd call #use instead of #request or #response.
I agree at some point we should drop the registry completely, but we decided to postpone it for now and not do this for v2.0. Maybe v3.0?

@mollerhoj
Copy link
Contributor Author

For some reason I can't commit on your branch in any way 🤔

Screenshot 2021-10-08 at 11 35 14 am

Could you cherry-pick or replicate this commit into your PR?

Done.

I'm reading through this gem, and I can't seem to understand what the point of this registry and the adapter registry is. Why not just add the middleware classes directly

This is mostly for legacy support, and you correctly pointed out is not necessary at all. You can simply add the middleware class, though in that case you'd call #use instead of #request or #response. I agree at some point we should drop the registry completely, but we decided to postpone it for now and not do this for v2.0. Maybe v3.0?

Okay, I see, thank you for the explanation.

@iMacTia
Copy link
Member

iMacTia commented Oct 8, 2021

Thanks @mollerhoj, but I think you accidentally force-pushed only my commit on your branch, I didn't include your change in my commit as well because it was supposed to be added 😄

I was quite surprised to realize I had to do register this middleware manually, as I didn't have to do that for the other middleware in core.
@mollerhoj
Copy link
Contributor Author

Ah, good catch! All good now?

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

All good! Thanks again for fixing this 🙌!
And thanks for using the main branch in your projects.
This type of "edge release" feedback in incredibly useful to us 🙏

@iMacTia iMacTia merged commit 5366029 into lostisland:main Oct 8, 2021
iMacTia pushed a commit that referenced this pull request Feb 16, 2022
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