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

Update dependency intl to ^0.18.0 #1202

Closed
wants to merge 4 commits into from
Closed

Update dependency intl to ^0.18.0 #1202

wants to merge 4 commits into from

Conversation

zeshuaro
Copy link

@zeshuaro zeshuaro commented Dec 21, 2022

📜 Description

Update dependency intl to ^0.18.0 - https://pub.dev/packages/intl.

💡 Motivation and Context

My project uses both sentry and intl. Right now, I'm unable to upgrade intl to ^0.18.0 in my project because sentry depends on intl: ^0.17.0 and the version resolution would fail.

💚 How did you test it?

Hoping GitHub Actions will verify that the new version of intl is still compatible.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@marandaneto
Copy link
Contributor

@zeshuaro why is it not possible? Sentry uses the ^ operator, so it should be fine to use the latest version as well.
Can you link us the error and why?

@zeshuaro
Copy link
Author

This is the error I'm getting, you can also see the build here.

Because no versions of sentry_flutter match >6.18.1 <7.0.0 and sentry_flutter >=6.18.0 <6.18.1 depends on sentry 6.18.0, sentry_flutter >=6.18.0 <6.18.1-∞ or >6.18.1 <7.0.0 requires sentry 6.18.0.
And because sentry >=6.11.0 depends on intl ^0.17.0, sentry_flutter >=6.18.0 <6.18.1-∞ or >6.18.1 <7.0.0 requires intl ^0.17.0.
And because sentry_flutter 6.18.1 depends on sentry 6.18.1 which depends on intl ^0.17.0, sentry_flutter ^6.18.0 requires intl ^0.17.0.
So, because appainter depends on both intl ^0.18.0 and sentry_flutter ^6.18.0, version solving failed.

My guess is that Pub classifies minor bumps in versions 0.x.x as breaking change, so it's not able to resolve the latest version of intl.

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2022

Codecov Report

Base: 90.03% // Head: 88.87% // Decreases project coverage by -1.16% ⚠️

Coverage data is based on head (b47429d) compared to base (a094100).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1202      +/-   ##
==========================================
- Coverage   90.03%   88.87%   -1.17%     
==========================================
  Files         110      121      +11     
  Lines        3483     3810     +327     
==========================================
+ Hits         3136     3386     +250     
- Misses        347      424      +77     
Impacted Files Coverage Δ
dio/lib/src/breadcrumb_client_adapter.dart 80.00% <0.00%> (ø)
dio/lib/src/failed_request_interceptor.dart 100.00% <0.00%> (ø)
dio/lib/src/sentry_transformer.dart 94.44% <0.00%> (ø)
dio/lib/src/tracing_client_adapter.dart 85.71% <0.00%> (ø)
logging/lib/src/extension.dart 100.00% <0.00%> (ø)
dio/lib/src/sentry_dio_extension.dart 76.92% <0.00%> (ø)
file/lib/src/sentry_file_extension.dart 80.00% <0.00%> (ø)
logging/lib/src/logging_integration.dart 90.47% <0.00%> (ø)
file/lib/src/sentry_file.dart 54.13% <0.00%> (ø)
dio/lib/src/sentry_dio_client_adapter.dart 100.00% <0.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -15,7 +15,7 @@ dependencies:
meta: ^1.3.0
stack_trace: ^1.10.0
uuid: ^3.0.0
intl: ^0.17.0
intl: ^0.18.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

There doesn't seem to be a breaking change: https://pub.dev/packages/intl/changelog

Suggested change
intl: ^0.18.0
intl: '>=0.17.0 <0.19.0'

Copy link
Author

Choose a reason for hiding this comment

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

I'm not suggesting the package has breaking changes, I was guessing that Pub treats minor bumps in versions 0.x.x as breaking, which does seem to be the case from here:

Although semantic versioning doesn’t promise any compatibility between versions prior to 1.0.0, the Dart community convention is to treat those versions semantically as well. The interpretation of each number is just shifted down one slot: going from 0.1.2 to 0.2.0 indicates a breaking change, going to 0.1.3 indicates a new feature, and going to 0.1.2+1 indicates a change that doesn’t affect the public API.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's strange because intl 0.17.0 (0.18.0 available)
it's a minor version but requires dart pub upgrade --major-versions.
Likely a bug in pub?
The suggestion won't work either because sentry won't be compatible with 0.19.0 once is released.

Copy link
Contributor

Choose a reason for hiding this comment

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

Likely related to flutter/flutter#117163

Copy link
Contributor

@marandaneto marandaneto Dec 21, 2022

Choose a reason for hiding this comment

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

Tag 0.18.0 is not created yet in the intl repo, so I can't check it for now either.
https://github.com/dart-lang/intl/tags

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added an entry to the changelog, but I think there might still be issues with flutter_localization from the issues you linked.

Indeed, https://github.com/flutter/flutter/blob/07c47dcc3d55be89cb3dbb3d10d5d65ed455c4bd/packages/flutter_localizations/pubspec.yaml#L11
They pinned the version, so if I bump, its incompatible as well.
Not sure what's the best approach here now.
@ueman ideas?
Instead of using ^0.18.0, we could do any, but that could lead to new problems in the future as well due to breaking changes, but it solves the problem right now.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think setting any would be the solution here, would definitely cause more issues later on. Maybe the solution is to simply wait for Flutter to fix things on their sides.

Copy link
Collaborator

@ueman ueman Jan 14, 2023

Choose a reason for hiding this comment

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

As said, I would suggest

Suggested change
intl: ^0.18.0
intl: '>=0.17.0 <0.19.0'

It's better than the current situation and even ^0.18.0 wouldn't be compatible with 0.19.0 as demonstrated by this PR. So, I think the suggestion is the way to go. It's a bit better than any, but, granted, only a bit.

In the future, intl should probably be vendored or just removed to avoid this problem. There's just one usage anyway, I believe.

Does disallowing 0.17.0 break usages on older Flutter versions?

Copy link
Author

Choose a reason for hiding this comment

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

Does disallowing 0.17.0 break usages on older Flutter versions?

I believe so based on the fact that we're required to make this current change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah IMO the bug is not here, let's see if they act quickly on dart-lang/pub#3747 otherwise we push @ueman suggested fix for now.
Vendoring the class would work too but I'd like to avoid it for now.

@marandaneto
Copy link
Contributor

@zeshuaro can you add an entry to the changelog? Thanks!

@zeshuaro
Copy link
Author

I've added an entry to the changelog, but I think there might still be issues with flutter_localization from the issues you linked.

@marandaneto
Copy link
Contributor

Closed in favor of #1247
Thank you all!

@zeshuaro zeshuaro deleted the patch-1 branch January 26, 2023 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants