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

fix(firebase_ui_auth): Error added in ProfileScreen in firebase_ui_auth 1.4.0 #10957

Merged
merged 3 commits into from May 17, 2023

Conversation

zachary-russell
Copy link
Contributor

@zachary-russell zachary-russell commented May 12, 2023

Description

firebase_ui_auth throws an error when building for version 1.4.0.:
../../.pub-cache/hosted/pub.dev/firebase_ui_auth-1.4.0/lib/src/screens/profile_screen.dart:393:19: Error: Cannot invoke a non-'const' constructor where a const expression is expected. Try using a constructor or factory that is 'const'. const Row( ^^^

Related Issues

Issue

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.

  • [ x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [ x] My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • [ x] All existing and new tests are passing.
  • [x ] I updated/added relevant documentation (doc comments with ///).
  • [x ] The analyzer (melos run analyze) does not report any problems on my PR.
  • [ x] I read and followed the Flutter Style Guide.
  • [ x] I signed the CLA.
  • [ x] 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.
  • [ x] No, this is not a breaking change.

@zachary-russell zachary-russell changed the title Fix Error: ../../.pub-cache/hosted/pub.dev/firebase_ui_auth-1.4.0/lib/src/screens/profile_screen.dart:393:19: Error: Cannot invoke a non-'const' constructor where a const expression is expected. Try using a constructor or factory that is 'const'. const Row( ^^^ Fix Error added in ProfileScreen in firebase_ui_auth 1.4.0 May 12, 2023
@zachary-russell zachary-russell changed the title Fix Error added in ProfileScreen in firebase_ui_auth 1.4.0 fix:(firebase_ui_auth) Error added in ProfileScreen in firebase_ui_auth 1.4.0 May 12, 2023
@zachary-russell zachary-russell changed the title fix:(firebase_ui_auth) Error added in ProfileScreen in firebase_ui_auth 1.4.0 fix(firebase_ui_auth): Error added in ProfileScreen in firebase_ui_auth 1.4.0 May 12, 2023
@lesnitsky
Copy link
Member

lesnitsky commented May 15, 2023

@zachary-russell analyzer fails with your change

firebase_ui_auth:
Analyzing ....

   info - lib/src/screens/profile_screen.dart:393:13 - Use 'const' with the constructor to improve performance. Try adding the 'const' keyword to the constructor invocation. - prefer_const_constructors

1 issue found.

@zachary-russell
Copy link
Contributor Author

@zachary-russell analyzer fails with your change


firebase_ui_auth:

Analyzing ....



   info - lib/src/screens/profile_screen.dart:393:13 - Use 'const' with the constructor to improve performance. Try adding the 'const' keyword to the constructor invocation. - prefer_const_constructors



1 issue found.

I think this issue is what caused the bug in the first place. I'll take a look and see if I can resolve it.

The prefer_const_constructors warning is ignored in this case because the LoadingIndicator widget used inside the children list of the Row does not have a const constructor. This is due to the fact that LoadingIndicator extends PlatformWidget, which doesn't have a const constructor, making it impossible to have a const constructor for LoadingIndicator. Since we can't make the LoadingIndicator constructor const, we have to ignore the warning for the specific Row constructor. This is an acceptable solution in cases like this, where making the constructor const is not feasible or possible.
@zachary-russell
Copy link
Contributor Author

zachary-russell commented May 15, 2023

@lesnitsky

The prefer_const_constructors warning is ignored in this case because the LoadingIndicator widget used inside the children list of the Row does not have a const constructor. This is due to the fact that LoadingIndicator extends PlatformWidget, which doesn't have a const constructor, making it impossible to have a const constructor for LoadingIndicator. Since we can't make the LoadingIndicator constructor const, we have to ignore the warning for the specific Row constructor. This is an acceptable solution in cases like this, where making the constructor const is not feasible or possible.

I'm still getting errors in the code:

info - lib/src/screens/profile_screen.dart:396:25 - Use 'const' literals as arguments to constructors of '@immutable' classes. Try adding 'const' before the literal. - prefer_const_literals_to_create_immutables

I don't know exactly how to resolve this and I seem to be going in circles. I'd appreciate a nudge in the right direction here :)

Are you aware of another way to solve it I'm definitely willing to give it a stab.

@russellwheatley
Copy link
Member

hey @zachary-russell, thanks for the PR. Do you mind changing it so the const is on children like so: https://github.com/firebase/flutterfire/pull/10963/files. I think that should work but if you could double check it builds on your end that would be great. Thanks 😄

@zachary-russell
Copy link
Contributor Author

zachary-russell commented May 16, 2023

Sure @russellwheatley, that's what was rejected on the initial commit I made 😸

I can confirm it works (both commits either make the List<Widget> a const or add in const for the items that aren't loading indicator). This seems to be just a bug on the linting/Melos that is a bit overzealous (or at least I can't figure out a way to fix it, the warnings are circular logic flows).

More than happy to help fix the linting rules as well if you cant point me in the right direction.

Copy link
Member

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

I think that ought to do it, thanks! 👍

@lesnitsky lesnitsky merged commit 12284b3 into firebase:master May 17, 2023
17 of 18 checks passed
@bkoznov
Copy link

bkoznov commented May 23, 2023

Is there a timetable/schedule for when this will be released to pub.dev ?

@firebase firebase locked and limited conversation to collaborators Jun 17, 2023
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.

None yet

4 participants