http icon indicating copy to clipboard operation
http copied to clipboard

feat: Add feature to parse multiple set-cookies

Open myConsciousness opened this issue 2 years ago • 2 comments

Hi amazing developers,

Perhaps this is already on the agenda before, but the http library combines multiple set-cookies in the response header into a comma-separated list that is set in the headers field. This is a rather complicated specification and not one that developers can easily handle.

Therefore, I have added the feature to parse set-cookies contained in headers to make it easier to handle multiple set-cookies. This modification is not disruptive and the headers field will continue to function as before.

However, in the future you need to reintegrate cookies field when you make the headers field an object, as was written in the BaseResponse TODO.

Also it was necessary to add universal_io to the dependencies to use Cookie object.

Thank you.

myConsciousness avatar Apr 19 '22 01:04 myConsciousness

Thanks for the PR! Some thoughts:

  • this is a highly used package, so we generally want to be very conservative about adding a new package dependency. You're adding package:universal_io - I assume to get the definition of the Cookie class? We'll likely have to remove the dep. It looks like package:universal_io exports the dart:io Cookie class in a dart:io context, and defines it's own class in other contexts. I don't know if that makes sense to do in this library, but fyi
  • adding @natebosch for review (and @brianquinlan for fyi, in case he has comments on the PR)

devoncarew avatar Apr 26 '22 00:04 devoncarew

Thanks for your feedback @devoncarew !

This is exactly what I was thinking too.

I used to get an error in static analysis when I uploaded a package I was developing that used dart:io to Pub.dev, and this is the reason for adding universal_io but now it seems I misunderstood the cause of that error.

And now I have scrutinized the universal_io implementation and found no reason to use universal_io instead of dart:io in this fix. I'm going to refactor my pull request.

Thank you.

myConsciousness avatar Apr 26 '22 01:04 myConsciousness

FYI this is related to https://github.com/dart-lang/http/issues/24 which proposes a different approach

vaind avatar Nov 02 '22 18:11 vaind

up ,there's still an issue about parsing multiple set-cookies now , all set-cookies will be covered by last one
hope this can be merged asap

XNORGATE avatar Apr 24 '23 15:04 XNORGATE