-
-
Notifications
You must be signed in to change notification settings - Fork 848
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
Replace the flutter_image package with RetryClient from the http package #894
Replace the flutter_image package with RetryClient from the http package #894
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnpryan I think this is a good approach to the lack of maintenance of flutter_image
. Removing a dependency is good practice when the usage is limited to a single use-case (here NetworkImageWithRetry). It increases the code footprint slightly, but not to a degree that is concerning.
It would introduce a breaking change that need to be noted in the change log @josxha
@josxha That would be great! Establishing an entry for version 0.13.0 and adding this change would suffice for me to approve the change (if the community agree with it). |
@kengu How should I proceed with the mentions? Are only committers counted or also code reviewers? |
Both collaborators and code authors I guess. |
I hope these are all the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think this is a good change. Removing dependencies that are use-case specific is important (a feature that has a hanging dependency). I think this is a good middle ground for this since most things use http we are simplifying the dependency chain. |
@josxha / @moonag I've done some testing with the example app and there are significant difference in behaviour between Behaviour without Behaviour with I think we should stick to flutter_image now that it is migrated to null-safety. We still need to wait for them to release a new version before we can release a new version of |
@kengu caching should be handled by your own implementation and have nothing to do with the image provider. Check the Readme section of flutter map for how to implement caching. Ping me if you would like me to help you (I handled that section) or make a sample project with and without flutter_image. If that does not make the caching consistent then ping me and I will look into it because then it would be a bug that I need to fix. |
@moonag I'm not able to do any changes to this PR, OP @josxha needs to do that. My point it that changing to this new implementation of Now that |
Nice to see that my suggestion has been implemented here already ^^ I just wanted to do that myself, so thanks @josxha for doing this yourself and sparing my time here :) |
Rebase onto master
I find @kengu's cache test very interesting. flutter_image clearly offers the better "out of the box" solution here. I have merged the pull request josxha#1 so that a potential release is not stuck with me. It looks good to me. |
I also find the cache test indeed very interesting. I also think, this should really be handled by the user. For this very reason, there already exist some libraries that cover this behavior. See https://github.com/JaffaKetchup/flutter_map_tile_caching for example. |
I think whatever is chosen, it's very key to make sure there's some documentation about it in the readme (and ideally suggest the alternatives). |
@ThexXTURBOXx My main reservation it that this change results in unexpected behaviour for current users of NetworkTileProvider. Even with proper warnings in changelog and readme, it is highly probable that someone is going to file a bug. |
I just wanted to chime in because I am the one to ping about caching. For this package caching is supposed to be handled by the user because every person needs a different solution. I have documented in the Readme turn-key disk/memory caching solutions. I suspect that the flutter_image package is providing memory caching. I think that is the package I use in the Readme as the memory caching solution. When using this pr please use one of the caching solutions in the Readme (which involves creating your own tile provider) to see if the issue persists. If it does persist it is an issue with this pr. It is likely people will see that as a bug and report it. If they do tell them to read the Readme. We can alleviate it by including the breaking change in the change log instructing people to read the caching section of the Readme. |
@moonag Actually, after some research, I found out that the Since it is undocumented behavior, it would be even better to move away from this old behavior to implementing this caching by ourselves using |
Hi all, |
@JaffaKetchup I already mentioned your plugin here :) |
That plug-in implements an even more turn key solution. It came as a result of us deciding any caching is going to be handled outside of flutter_map. That plugin was created and I added documentation to the Readme. To think of it I should also add mention to that plug-in in the Readme. Removal of flutter_image from flutter_map is the right decision. The only value it provides is NetworkImageWithRetry and it also exhibits unexpected caching behavior as it is caching where it maybe shouldn't be. Users of flutter_map should use a proper caching image provider. The change in behavior will probably break some people's projects but it will be for the better; and they will know about the breaking change in a change log entry with an explanation of the change and how to fix it by reading the Readme and or using a flutter_map plugin or using a caching image provider they create in house or on pub.dev |
@ThexXTURBOXx It seems like you did indeed! GitHub was hiding some comments from me, so I couldn't see it without clicking to show more. :) @moonag I made a pull request (#869) a few weeks back adding my plugin and removing the information about self-implementation of caching that you added in (in PR #841) because it seemed less useful with the new plugin. If you want to add your guide back in, just let me I know and I can open a new PR if you don't want to do it. However, I also agree that |
@JaffaKetchup I think we should keep the docs for self-implemented caching and the little paragraph I had explaining how the chain of ImageProvider->TileProvider-> what gets displayed on the map works to make sure that caching makes sense to people and isn't just a black box. Or if people want to use flutter_image, or some other image provider they are already using, have made in house, or prefer more than the black box plugin. It is just a few lines of code and it will still work as long even if the caching plugin for flutter_map isn't maintained anymore. My personal preference for dependencies is less is more. Hanging features and things that are user dependent or already have a huge ecosystem (like caching ImageProviders in flutter) CAN have a black box solution for flutter_map but it should be clearly documented that you can cache without an extra black_box dependency or use an image provider you already are using. I think options are good but just telling people to use a black box solution to something like caching is dangerous when it is can be easily handled by 5 lines of code and an existing well maintained image provider. |
As far as I know
Glad to hear about |
@ThexXTURBOXx Nothing more than switching between the implementations. @moonag If we are going this route, the broader question then becomes what the real value of having two network providers? The naming of I think the new implementation of NetworkTileProvider could be renamed to something like |
Yeah we removed it because caching is such a user specific thing we wanted to allow for the user to choose how or if they cache their image tiles. Also that dependency was heavy and relied on a lot of other dependencies. Removing it gave a choice of how people want to cache and made flutter_map significantly more lightweight. I added the docs on how to create a tile provider with whatever image provider (that caches or doesn't cache) you want at that time to document the change. |
@kengu The expectation should be that NO built in tile providers provide caching. Some might still provide memory caching. There is a built in flutter image provider that might be used in one of the tile providers that does memory caching. |
I don't disagree with that, less is more here, and one less dependency is a good thing. But I want to make sure we help the community to understand this through consistent naming of code, not readme alone (which many do not read that carefully). |
@moonag, did you want me to add back your instructions then, perhaps as a 'if you don't want another package in your project, then you can do this...'? If so, I'll open a new PR at some point soon. |
@JaffaKetchup yeah let's open a PR to add the docs back and also mention that there are no built in tile providers that provide caching and also look at modifying this PR to include changes to the built in tile providers naming to make it not sound like they cache. |
Ok, I'm making a new PR now. |
I have further changes my PR to reflect the new information I have found looking through the source code. |
Since flutter_image seems to have a very low priority from the flutter development team, I followed this tip and replaced the package with the RetryClient from the http package. I tested my pull request with the example app and it worked fine.
The PR would also make flutter_map independent of flutter_image in the future.