Skip to content

Conversation

@creativecreatorormaybenot
Copy link

@creativecreatorormaybenot creativecreatorormaybenot commented Jun 2, 2021

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

This fixes the breaking change I explained in #302 (comment).

⤵️ What is the current behavior?

When you run pub upgrade with flutter_cache_manager: ^3.0.0 and used 3.0.0 before, your code breaks because you might have imported dart:io (or universal_io) and flutter_cache_manager in the same file.

🆕 What is the new behavior (if this is a feature change)?

Removed the export of package:file. Instead, users can import it when needed.

What should be done instead?

@sidrao2006 said that you have to add a direct package:file depedendency if you want to annotate types without his PR (this reverts #302), however, that is not true.

You can import package:file/file.dart, even when it is a transitive dependency.

Migration

So the migration is simply adding:

import 'package:file/file.dart' show File;

Whenever you want to use that File class.

💥 Does this PR introduce a breaking change?

Technically yes, but it fixes the previous breaking change.

🤔 Checklist before submitting

  • All projects build
  • [x ] Follows style guide lines (code style guide)
  • Relevant documentation was updated
  • Rebased onto current develop

@sidrao2006
Copy link
Contributor

@creativecreatorormaybenot for some reason importing transitive dependencies isn't allowed in packages

@creativecreatorormaybenot
Copy link
Author

@sidrao2006 ok, it makes sense in packages. But in that case I would prefer to make the dependency explicitly visible, or not? 🤔

The reason I said it was a breaking change is that in our apps, it would not compile anymore, unless we add:

import 'package:flutter_cache_manager/flutter_cache_manager.dart' hide File;

Because of the mentioned conflict.

@sidrao2006
Copy link
Contributor

I completely understand and apologize for the change. When I made the change, the dart:io conflict didn't occur to me.
Sorry for the inconvenience caused.

@creativecreatorormaybenot
Copy link
Author

@sidrao2006 No worries. The change itself is fine as well (although I would say it generally does not make most sense to export other packages from your package), but it should have bumped the major.

@renefloor renefloor mentioned this pull request Jun 3, 2021
4 tasks
@renefloor
Copy link
Contributor

Slightly different fix from PR #324 is released as 3.1.1

@renefloor renefloor closed this Jun 3, 2021
@creativecreatorormaybenot
Copy link
Author

@renefloor I thought about this approach as well, but I think it is a bad idea.

You are essentially proxying import 'package:file/file.dart (so you have a library in your package which has the sole purpose of proxying another library). The reason this is also dangerous is that users will not have an explicit dependency on file, which means that they can be hit by breaking changes without being able to know why.

Now, you are required to publish a breaking change / major version update whenever file has a major version update / breaking change because you are exporting it.

@sidrao2006
Copy link
Contributor

sidrao2006 commented Jun 3, 2021

@creativecreatorormaybenot , only File is exported from the package. So, a major version would have to be published only if there are massive changes in File which is highly unlikely. Even in such a case, several major refactors would have to be made in flutter_cache_manageras well. So, then it would make sense to release a major version (or minor, as needed)

@creativecreatorormaybenot
Copy link
Author

@sidrao2006 yes that is true.

@renefloor
Copy link
Contributor

Indeed, if for example file.read() would change then the cache_manager package needs a breaking change anyhow. In general it is not a good idea to export other packages, but it's also weird that you get a File, but can only do var file =.. and not File file = ... I'm also considering migrating to cross_file, but I'll have to study the differences.

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