[go_router_builder] Migrate to Element2 API and update dependencies #9649
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the go_router_builder package to the Element2 API, updating dependencies and improving test code formatting. The changes are well-structured and consistently applied across the codebase. The migration to the new API is crucial for maintaining compatibility with the latest versions of analyzer, build, and source_gen.
| if (element.isRequired) { | ||
| if (element.type.nullabilitySuffix == NullabilitySuffix.question && | ||
| _pathParams.contains(element.name)) { | ||
| _pathParams.contains(element.displayName)) { |
There was a problem hiding this comment.
The migration to Element2 API involves changes to how parameter names are accessed. Ensure that the display name is correctly used throughout the codebase to maintain consistency and prevent unexpected behavior.
| @@ -60,14 +60,14 @@ class ShellRouteConfig extends RouteBaseConfig { | |||
|
|
|||
| @override | |||
| Iterable<String> classDeclarations() { | |||
| if (routeDataClass.unnamedConstructor == null) { | |||
| if (routeDataClass.unnamedConstructor2 == null) { | |||
There was a problem hiding this comment.
The migration to Element2 API involves changes to how constructor elements are accessed. Consider updating the error message to reflect the new API usage and improve clarity for developers debugging potential issues related to constructor access.
| final String generated = formatter.format(results.join().replaceAll('\n', '')); | ||
| final String expected = formatter.format(expectResult.replaceAll('\n', '')); | ||
| expect(generated, equals(expected)); |
There was a problem hiding this comment.
The comparison logic has been updated to improve robustness against formatting variations. Consider adding a comment explaining why the replaceAll calls are necessary to improve code understanding and maintainability.
|
Mac_arm64 custom_package_tests master and Mac_arm64 custom_package_tests stable are passed, but Windows_x64 custom_package_tests master is failed. |
|
Fix test code to support windows path separator. 96d68f9 |
| } | ||
| } | ||
|
|
||
| InfoIterable _generateForAnnotatedElement( | ||
| Element element, | ||
| Element2 element, |
There was a problem hiding this comment.
looking at the doc https://pub.dev/documentation/analyzer/latest/dart_element_element/Element2.html
should we keep the original class name
There was a problem hiding this comment.
@chunhtai
Element2 will be deprecated starting with analyzer v8.0.0. Since analyzer: “>=7.4.0 <8.0.0” is set, Element2 is not deprecated.
https://pub.dev/documentation/analyzer/7.7.1/dart_element_element2/Element2-class.html
https://pub.dev/documentation/analyzer/8.0.0/dart_element_element/Element2.html
Other libraries are also replacing Element2 in the same way.
There was a problem hiding this comment.
can we pin analyzer 8.0.0 and above directly?
There was a problem hiding this comment.
@chunhtai
It will be difficult as it was not announced in advance. Specifying ^8.0.0 will break compatibility with other packages.
https://github.com/dart-lang/mockito/blob/v5.4.5/pubspec.yaml
https://github.com/google/json_serializable.dart/blob/json_serializable-v6.10.0/json_serializable/pubspec.yaml
https://github.com/rrousselGit/freezed/blob/freezed-v3.2.0/packages/freezed/pubspec.yaml
| final String routeData = _annotations[type]!; | ||
| if (element is! ClassElement) { | ||
| if (element is! ClassElement2) { |
There was a problem hiding this comment.
| throw InvalidGenerationSourceError( | ||
| 'The ShellRouteData "$_className" class must have an unnamed constructor.', | ||
| element: routeDataClass, | ||
| ); | ||
| } | ||
|
|
||
| final bool isConst = routeDataClass.unnamedConstructor!.isConst; | ||
| final bool isConst = routeDataClass.unnamedConstructor2!.isConst; |
There was a problem hiding this comment.
|
@chunhtai https://github.com/dart-lang/source_gen/blob/source_gen-v3.0.0/source_gen/pubspec.yaml |
close flutter/flutter#170832
close flutter/flutter#171003
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3