Skip to content
This repository has been archived by the owner on Feb 6, 2021. It is now read-only.

Remove forbidden headers #30

Merged
merged 1 commit into from
Sep 1, 2019

Conversation

jonasfj
Copy link
Contributor

@jonasfj jonasfj commented Aug 12, 2019

@StorminGorman, do you think this would fix issue google/googleapis.dart#73

@jonasfj
Copy link
Contributor Author

jonasfj commented Aug 12, 2019

Test passed locally in chrome and dart-vm.

@jonasfj jonasfj requested a review from sigurdm August 12, 2019 15:14
@StorminGorman
Copy link

Looks good to me. I haven't found a reason in any dart projects to use a conditional import so that was cool seeing that trick. If any more http/io differences occur I believe it would be best to find a polymorphic solution.

I am curious though and I don't know off hand, would the same errors occur if setting the content-length, and user-agent headers for the simpleUpload() request that occurs during a media upload? That would require a little more looking into and if that is the case then the same forbidden headers should be removed from the code for that as well.

https://github.com/dart-lang/discoveryapis_commons/blob/08d4ef8d78da6f318e8eb0090542681fb8812edd/lib/src/clients.dart#L182

// ignore: uri_does_not_exist
if (dart.library.html) 'forbidden_headers_js.dart'
// ignore: uri_does_not_exist
if (dart.library.io) 'forbidden_headers_io.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think conditional imports is a too big hammer for this.

You can get the necessary information by:

const forbiddenHeaders = bool.fromEnvironment('dart.library.html') ? <String>['user-agent', 'content-length'] : <String>[];

@jonasfj jonasfj merged commit d9ef904 into dart-archive:master Sep 1, 2019
@jonasfj jonasfj deleted the fix-forbidden-headers branch September 1, 2019 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants