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

Remove multipart middleware and all its documentation and tests #1357

Merged
merged 3 commits into from
Jan 3, 2022

Conversation

iMacTia
Copy link
Member

@iMacTia iMacTia commented Jan 2, 2022

Description

Remove multipart middleware and all its documentation and tests.
Multipart middleware has been moved to a separate repo.

Todos

List any remaining work that needs to be done, i.e:

  • Tests
  • Documentation

@iMacTia iMacTia self-assigned this Jan 2, 2022
@olleolleolle
Copy link
Member

olleolleolle commented Jan 3, 2022

Perhaps this could be in UPGRADING.md or something, how to include the new gem in the Gemfile, and how to include the file required.

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Removal looks ace, I made a note about "perhaps this one is a thing to teach somewhere, for people upgrading".

@iMacTia
Copy link
Member Author

iMacTia commented Jan 3, 2022

@olleolleolle good point, I agree they deserve their own section for clarity. I've just added that 👍

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

LGTM!

@olleolleolle olleolleolle merged commit 88d16b7 into main Jan 3, 2022
@iMacTia iMacTia deleted the remove-multipart-middleware branch January 3, 2022 09:21
@ahmedakef
Copy link

@iMacTia @olleolleolle it is listed in the change log of v 2.0 while it is merged into v 1.9.0 release https://github.com/lostisland/faraday/releases/tag/v1.9.0

it this expected ?

@iMacTia
Copy link
Member Author

iMacTia commented Feb 28, 2022

@ahmedakef that's correct, this change was applied to both version branches.
It was backported to 1.9.0 as part of #1367, while this PR made the change to the main branch (v2.0)

@ahmedakef
Copy link

@iMacTia but isn't this a breaking change ?

@iMacTia
Copy link
Member Author

iMacTia commented Feb 28, 2022

@ahmedakef oh no we'd never do that!

In #1367 we removed the middleware code from Faraday, but we added both faraday-multipart and faraday-retry as direct dependencies, which basically "replaces" the removed code and provides backwards compatibility.
The idea is that you can upgrade to 1.9.0 and everything should still work exactly the same.

The real breaking change is in 2.0 where we remove those dependencies, which mean you'll now need to manually add them to the Gemfile if you want to use them.

In fact, the changelog for 1.9.0 says "Use external multipart and retry middleware", while the one for 2.0 is "Remove multipart middleware and all its documentation and tests".

@ahmedakef
Copy link

@iMacTia thank you so much for the explain
the breaking change for me was that I depend on google-api-ruby-client which call multipart here using:

require 'faraday/request/multipart'

and I have to fork and change it to this to work:

require 'faraday/multipart'

I already solved my problem, just telling you in case it cause problem to others

@iMacTia
Copy link
Member Author

iMacTia commented Feb 28, 2022

Ah thanks for reporting this @ahmedakef.
That seems a problem on the google-api-ruby-client gem though, because there's no need for requiring middleware thanks for Faraday's dependency autoloading feature.
Removing the require should make the code working across both versions. You can try this on your fork and then open a PR on their gem if it works as expected (as it should, unless something else is causing that not to work for any reason).

@iMacTia
Copy link
Member Author

iMacTia commented Feb 28, 2022

Ah, I see they're referencing it directly here: https://github.com/googleapis/google-api-ruby-client/blob/v0.8-main/lib/google/api_client/request.rb#L326

That feels really wrong, they're completely bypassing the middleware this way and calling it explicitly, it doesn't really make sense...

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.

3 participants