Skip to content

Remove multipart middleware and all its documentation and tests#1357

Merged
olleolleolle merged 3 commits intomainfrom
remove-multipart-middleware
Jan 3, 2022
Merged

Remove multipart middleware and all its documentation and tests#1357
olleolleolle merged 3 commits intomainfrom
remove-multipart-middleware

Conversation

@iMacTia
Copy link
Copy Markdown
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 requested a review from olleolleolle January 2, 2022 21:08
@iMacTia iMacTia self-assigned this Jan 2, 2022
@olleolleolle
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

@iMacTia but isn't this a breaking change ?

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

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