-
Notifications
You must be signed in to change notification settings - Fork 87
Update method signature of lookupResolvedPackageUris
#1633
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
Conversation
@override | ||
Future<UriList> lookupResolvedPackageUris( | ||
String isolateId, List<String> uris) async { | ||
Future<UriList> lookupResolvedPackageUris(String isolateId, List<String> uris, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update vm_service version to match? We need to require at least the minimum version that contains this new API or webdev users can end up using older vm_service without the parameter...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkonyi I found this PR in dart SDK introducing the change but it does not update the vm_service package version - am I right that we need to add a constraint to this PR to depend on that min version to prevent webdev users from pulling new dwds with old vm_service?
dart-lang/sdk@d6665ae#diff-60648d19c0fe66d08d01fb7a449378bb80241a95601fe86b3d34962586abb886
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkonyi if you would like to update the vm_service version later I think it is ok but we cannot publish dwds until that happens and we add the constraint. @elliette still can submit this PR I think, and both vm_service and dwds would be unpublished versions in g3 until then. I hope that I am not wrong:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkonyi addition - I believe it should be a major version update for vm_service for this all to work out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this patched in g3 first and then I'll do a major release. Unfortunately, a lot of downstream dependencies need to be updated and released with new versions first.
@override | ||
Future<UriList> lookupResolvedPackageUris( | ||
String isolateId, List<String> uris) async { | ||
Future<UriList> lookupResolvedPackageUris(String isolateId, List<String> uris, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkonyi I found this PR in dart SDK introducing the change but it does not update the vm_service package version - am I right that we need to add a constraint to this PR to depend on that min version to prevent webdev users from pulling new dwds with old vm_service?
dart-lang/sdk@d6665ae#diff-60648d19c0fe66d08d01fb7a449378bb80241a95601fe86b3d34962586abb886
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (if we confirm that checking this in and not publishing until vm_service version update and adding the constraint is ok.)
Yeah, we'll need to publish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but we can't publish yet. Maybe we should hold off landing this until we're ready to publish.
@override | ||
Future<UriList> lookupResolvedPackageUris( | ||
String isolateId, List<String> uris) async { | ||
Future<UriList> lookupResolvedPackageUris(String isolateId, List<String> uris, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this patched in g3 first and then I'll do a major release. Unfortunately, a lot of downstream dependencies need to be updated and released with new versions first.
Update on this: we're going to hold off on submitting it until the |
See: #1632