Migrate to null safety #93
Conversation
This cycle only exists in dev dependencies, I would use dependency overrides to force the version of package:test. This is what glob, args, etc are doing. The test package should not depend on a version range of its deps that allows both migrated and unmigrated versions (nor should any package), that seems very risky. |
lib/discovery.dart
Outdated
| @@ -2,6 +2,8 @@ | |||
| // for details. All rights reserved. Use of this source code is governed by a | |||
| // BSD-style license that can be found in the LICENSE file. | |||
|
|
|||
| // @dart=2.7 | |||
|
|
|||
| @Deprecated("Use the package_config.json based API") | |||
There was a problem hiding this comment.
We should do the breaking version and delete these libs.
There was a problem hiding this comment.
Seems the analyzer is still using the deprecated API.
There was a problem hiding this comment.
Is that because they need to use it or just haven't migrated?
There was a problem hiding this comment.
They are reading .packages files directly, then using the legacy functionality in this package to parse the file into a map. That's not supported directly by the new API. You can read a .packages file into a PackageConfig object using
loadPackageConfig(file, preferNewest: false);The plain map from name to package root isn't supported any more because it makes no sense for package_config.json files.
Sam is working on it now: dart-lang/sdk#44116
| Uri location, | ||
| Future<Uint8List /*?*/ > loader(Uri uri) /*?*/, | ||
| void onError(Object error) /*?*/, | ||
| Future<Uint8List?> loader(Uri uri)?, |
There was a problem hiding this comment.
It is pretty weird to have a nullable required positional argument like this... should we consider changing the api?
It seems like the only required argument should be location really and the others should be optional named args.
There was a problem hiding this comment.
It's internal API.
I usually don't bother with optional parameters for internal API, it's not there to be ergonomic, just to be clear and precise.
lib/src/package_config_json.dart
Outdated
| String /*?*/ packageUri; | ||
| String /*?*/ languageVersion; | ||
| Map<String, dynamic> /*?*/ extraData; | ||
| Package? parsePackage(Map<String, dynamic> entry) { |
There was a problem hiding this comment.
We have been going with Map<String, Object?> for all the other packages because it makes the potential error conditions more explicit via explicit casts instead of dynamic calls.
There was a problem hiding this comment.
I'll remove every instance of dynamic. And good riddance :)
This is the null safe, non-deprecated API for package_config.json file manipulation.
|
Used dependency overrides and made it a 2.0.0 version without deprecated APIs. Still can't be tested because the analyzer depends on the removed APIs. |
|
Tested, landed, will publish. |
|
Curious what the status on publishing here is? I notice the build is broken but maybe this expected? |
|
This had been blocked from being pulled into the SDK because of analyzer usage of a deprecated API. That was migrated to a @lrhn - is this ready to get updated in the SDK? Is it still blocked by the analyzer What is the next step to push on here? |
|
I think it's fine to pull into the SDK as-is. We'll fix the src/ dependency at some later point. |
* Migrate non-deprecated libraries to null safety. * Major version increment, removing deprecated APIs. This is the null safe, non-deprecated API for package_config.json file manipulation. Also address dart-lang/package_config#86.
Only migrated the non-deprecated code, as an extra incentive to get people off deprecated code.
Did not update the version number to 2.0.0-nullsafety.0 because pkg:test depends on null-safety<2.0.0.
It should be upgraded to depend on >=1.9.3 <3.0.0, so we can release a pkg:package_config 2.0.0 without the deprecated code.
Until then, we're stuck in package:test-recursive-dependency-hell.