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

'User-Agent' Header Generation & More #1294

Merged
merged 13 commits into from
Jul 11, 2022
Merged

'User-Agent' Header Generation & More #1294

merged 13 commits into from
Jul 11, 2022

Conversation

JaffaKetchup
Copy link
Member

@JaffaKetchup JaffaKetchup commented Jun 29, 2022

Fixes #1292. Includes breaking changes. Will require significant documentation changes.

This PR's main aim is to add a userAgentPackageName parameter to TileLayerOptions, which can be automatically passed to TileProviders for simplicity - developers do not have to specify tile providers if it is the default provider, just to add this parameter. This must then be combined with other headers correctly, resulting in one single correct 'User-Agent', and any other necessary headers.
There is also a 'lower level' headers implementation to TileProviders, so custom headers can be passed if needed when TileLayerOptions is not in use, for whatever reason.

At the same time, this deprecates NonCachingNetworkTileProvider in favour of NetworkNoRetryTileProvider, as the old name was misleading. Also in this refactoring, TileProviders lost their const ability. This will not affect developers that do not override the default provider, or did not include the const keyword when doing so - other developers will see breaking changes here.

In other news:

  • Some of the ints have been changed to Durations, as in the TODO list. This is a breaking change.
  • The _positionedForOverlay method has had it's TODO solved as well, by removing unnecessary multiplication by one. Note that this overlaps with [v2.1.0] Fixes For Polar Projection #1295, and will cause merge conflicts.
  • Other TODOs have been removed, because they were not getting fixed or they were not worth fixing.
  • Old deprecations (placeholderImage and attributionBuilder) have been completely removed.

Improved headers implementation
Deprecated `NonCachingNetworkTileProvider` in favour of `NetworkNoRetryTileProvider`
Updated example pages with changes
Improved documentation
@JaffaKetchup JaffaKetchup requested a review from ibrierley June 29, 2022 17:35
@JaffaKetchup JaffaKetchup changed the title 'User-Agent' Header Generation 'User-Agent' Header Generation & More Jun 29, 2022
@JaffaKetchup
Copy link
Member Author

It appears this is working. Interestingly, the original 'User-Agent' is being sent as well. Will this be an issue @pnorman?

image

@pnorman
Copy link

pnorman commented Jun 29, 2022

A user-agent of Dart/2.17 (dart:io), flutter_map (...) is acceptable. If it's sending multiple of the same header, that would be a problem.

@JaffaKetchup
Copy link
Member Author

Well I'm not sure if there are multiple headers being sent or just one. It's showing as an array, which suggests to me that there are two, separate user-agents being sent.

Sorry for the pinging @pnorman, but can you provide some insight as to how these user-agents are checked?

Also note, on debug browser, the User-Agent appears to be the browser's and nothing else.

@pnorman
Copy link

pnorman commented Jun 30, 2022

There is only one user-agent header, so I'm not sure which it'd send if it's an array.

The user-agents are checked on the CDN

@ibrierley
Copy link
Contributor

I suspect it probably just concatenates all those in an Array. So the ideal would probably be to overwrite the first element, if another is set, but I suspect that's a bit too far removed from our control.

@JaffaKetchup
Copy link
Member Author

@ibrierley I've been researching, and it might be possible. I'll see what I can do in terms of using HttpOverrides.

Fixed usage of `NetworkTileProvider`
Fixed deprecation notice invalid symbol reference
@JaffaKetchup
Copy link
Member Author

Waiting on dart-lang/sdk#49382 now.

Simplified `_positionedForOverlay` (solved TODO)
Improved maintainability
Improved documentation
@pnorman
Copy link

pnorman commented Jul 1, 2022

I suspect it probably just concatenates all those in an Array

Well, it sends a string. If the string is a concatenation of all the array elements it would work

@JaffaKetchup
Copy link
Member Author

Interesting @pnorman. Would prefer to find a way to remove the old agent, but it's proving difficult.
Are you confirming that it's sending a string and concatenation, or not? I'm struggling to understand, sorry.

@pnorman
Copy link

pnorman commented Jul 1, 2022

Headers are strings. I don't know what it's sending, but it's not sending an array, as there are no types in headers.

@JaffaKetchup
Copy link
Member Author

Yeah, I guessed/thought/knew as much. It's probably joining them, thinking about it - most servers/code would be sensible enough to not just throw away data.

I'll keep trying to see if I can get my idea to work, but if it takes too long, I'll give up and just go back to the two headers.

@JosefWN
Copy link
Contributor

JosefWN commented Jul 1, 2022

If you want you can merge my additional fixes in my PR into yours, and I can close my PR. Would avoid the merge conflicts.

@JaffaKetchup
Copy link
Member Author

Thanks for the offer, but I'd prefer not to. Would make this PR just very big, and you'd lose some credit in the process. The conflicts shouldn't be too hard to iron out.

@@ -1,9 +1,19 @@
import 'package:flutter/widgets.dart';
import 'package:universal_io/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.

Why is this new dependency needed?

pubspec.yaml Outdated
@@ -21,6 +21,7 @@ dependencies:
proj4dart: ^2.0.0
tuple: ^2.0.0
vector_math: ^2.1.0
universal_io: ^2.0.4
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@ibrierley
Copy link
Contributor

Hopefully I should get a bit of time at the weekend to check this. Couple of questions in meantime...
There was a note about universal_io earlier and why it's needed...is this an issue for people ?
Are there any other issues on NonCachingNetworkTileProvider being removed...are we fairly happy there's no missing functionality (I know we thought this provider didn't actually stop caching anyway...)

@JaffaKetchup
Copy link
Member Author

Wouldn't worry about checking it just yet, it still doesn't 100% work.
In terms of universal_io, I needed it to keep web support when using things from 'dart:_http', which can usually only be accessed through 'dart:io', which doesn't support web.
There is no lost functionality, only renaming. Everything that there was before can still be used, just with a different name and description.

@JaffaKetchup
Copy link
Member Author

Just wrote this: dart-lang/sdk#49382 (comment).

Added custom `ImageProvider`s
Reduced reliance on 'universal_io' library
Prepared for review
@JaffaKetchup JaffaKetchup marked this pull request as ready for review July 9, 2022 09:09
@JaffaKetchup
Copy link
Member Author

JaffaKetchup commented Jul 9, 2022

@ibrierley This is now ready for review.

I've reduced the reliance on 'universal_io', but it is still necessary to allow the library to compile for the web.

EDIT: Just found a compilation bug for the web, which needs to be fixed.

@JaffaKetchup JaffaKetchup marked this pull request as draft July 9, 2022 09:11
Seperated tile_provider.dart for web and other plaforms
Removed 'universal_io' dependency
Updated CHANGELOG
@JaffaKetchup JaffaKetchup marked this pull request as ready for review July 9, 2022 10:34
@JaffaKetchup
Copy link
Member Author

Ah, finally fixed it. Needed to remove the dependency on 'universal_io' in the end, it was not helping. Unfortunately, this has made the code less organised, as two 'tile_provider.dart' files are needed now.

Copy link
Contributor

@ibrierley ibrierley left a comment

Choose a reason for hiding this comment

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

This all tests fine for me. If anyone relies on things like user_agents in requests somehow I would test this (maybe give a couple of days for anyone to test and scream before merging :D?)

@JaffaKetchup
Copy link
Member Author

OK, I'm happy to wait for a couple of days, just in case anyone has any feedback.

I've prepared the documentation for v2 as well, so that's ready to go on release. Migration instructions have also been provided.

@JaffaKetchup JaffaKetchup merged commit f55e156 into fleaflet:master Jul 11, 2022
@JaffaKetchup
Copy link
Member Author

@greensopinion Looking through your plugin for vector tiles specifically, as this brought that to the forefront of my mind as I was editing the docs, you might need to make some changes to your custom tile providers to reflect these changes. Specifically https://docs.fleaflet.dev/usage/layers/tile-layer/tile-providers and https://docs.fleaflet.dev/plugins/making-a-plugin/creating-new-tile-providers#extending-the-base-tileprovider. Happy to help if you can't understand something!

This also applies to anyone else with custom tile providers, but this is quite a major plugin that uses them. If anyone's looking for info about flutter_map_tile_caching v5, the next prerelease will support flutter_map v2.

@greensopinion
Copy link
Contributor

@greensopinion Looking through your plugin for vector tiles specifically, as this brought that to the forefront of my mind as I was editing the docs, you might need to make some changes to your custom tile providers to reflect these changes.

@JaffaKetchup Thanks for the heads-up! Great to see more improvements to this library!

The vector_map_tiles plug-in doesn't use the TileProvider class of flutter_map because that one is specific to raster images. Instead, we have VectorTileProvider that is specific to vector tiles.

I've checked out this PR and run the example app. Everything appears to build and run without error. Not sure if you had something else in mind, but from what I can gather no changes are needed in the vector_map_tiles plug-in. Let me know if I'm missing something!

@JaffaKetchup
Copy link
Member Author

JaffaKetchup commented Jul 12, 2022

@greensopinion No problem!

In that case, I think the only thing to do is implement the 'User-Agent' header in your library, specifically in your custom layer options. You can use the implementation here for inspiration, if you need it.

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.

Include appropriate user-agent header when making map requests
6 participants