Skip to content

Register json middleware#1331

Merged
iMacTia merged 1 commit intolostisland:mainfrom
mollerhoj:patch-1
Oct 8, 2021
Merged

Register json middleware#1331
iMacTia merged 1 commit intolostisland:mainfrom
mollerhoj:patch-1

Conversation

@mollerhoj
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Ah, good catch! All good now?

Copy link
Copy Markdown
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