Skip to content

Conversation

rrousselGit
Copy link
Contributor

@rrousselGit rrousselGit commented Feb 22, 2022

Description

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@Salakar Salakar changed the title feat: Update error handling to preserve stackTraces feat: refactor error handling to preserve stack traces on platform exceptions Feb 24, 2022
@Salakar Salakar changed the title feat: refactor error handling to preserve stack traces on platform exceptions feat(*): refactor error handling to preserve stack traces on platform exceptions Feb 24, 2022
@Salakar Salakar merged commit 6ac77d9 into master Feb 24, 2022
@Salakar Salakar deleted the @rrousselGit/refactor-error-handling branch February 24, 2022 16:18
@ben-xx
Copy link

ben-xx commented Feb 24, 2022

@rrousselGit Hey Rémi, should min Dart SDK version be moved from 2.12 to 2.16 on pub.dev with this change?

Error/throwWithStackTrace.html was added in Dart 2.16 I believe.

After updating firebase_messaging to 11.2.7 (which moves firebase_messaging_platform_interface from 3.1.3 to latest 3.2.0) I started getting:

/C:/Users/Blah/AppData/Local/Pub/Cache/hosted/pub.dartlang.org/firebase_messaging_platform_interface-3.2.0/lib/src/method_channel/utils/exception.dart:13:11: Error: Member not found: 'Error.throwWithStackTrace'.
    Error.throwWithStackTrace(exception, stackTrace);
          ^^^^^^^^^^^^^^^^^^^

Moving back down to 3.1.3 in pubspec.yaml avoided the above:

dependency_overrides:
  firebase_messaging_platform_interface: 3.1.3

@dreamer2q
Copy link

This changes has broken people whose Dart SDK < 2.16 and make people disappointed.

@rrousselGit
Copy link
Contributor Author

The pubpsec of the changed package was correctly updated to mention >= 2.16 (see https://pub.dev/packages/firebase_core/versions)

It seems like pub isn't picking up transitive dependencies constraints thought

We can update all packages to explicitly require 2.16 (even if they weren't directly changed by this PR) if needed. Although it's not going to change the fact that you'd need Dart >= 2.16

@ben-xx
Copy link

ben-xx commented Feb 25, 2022

@rrousselGit Awesome! Thanks Rémi for the update & work on firebase changes / new Error API. Much appreciated.

@@ -9,7 +9,7 @@ false_secrets:
- example/**

environment:
sdk: ">=2.12.0 <3.0.0"
sdk: ">=2.16.0 <3.0.0"

Choose a reason for hiding this comment

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

You change the firebase_core min sdk to 2.16 with version 1.12.0. But this version has already been published with min sdk 2.12 on pub.dev. This does not make sense and thus resulted in unexpected compiling errors with dart sdk < 2.16.

The correct way (in my eyes) is to bump all packages to a higher version and make them dependent on a higher firebase_core version.

Choose a reason for hiding this comment

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

That is to say, there is nothing wrong with pub.dev.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pub should handle transitive dependencies too.

Choose a reason for hiding this comment

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

As far as I know, flutter is fetching packages from pub.dev and these packages once released are snapshots (which means they are unmodified). The newly released packages still rely on the old published firebase_core (which require sdk > 2.12) and the packages themselves still require sdk > 2.12, so there no change in sdk requirement but the actual code uses feature provided by sdk 2.16.

In short, you cannot expect a released package to upgrade its min sdk requirement, do you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Published packages don't have to update their sdk requirement.

It's about pub checking transitive dependencies on installation to verify that they all respect sdk constraints.

@firebase firebase locked and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants