Initial version of package:extension_discovery #129
Conversation
|
DBCs:
|
| It is the responsibility package that can be extended to validate extensions, | ||
| decide when they should be enabled, and documenting how such extensions are | ||
| created. |
There was a problem hiding this comment.
should we add here that it is also the responsibility of the package that will be extended to detect when the cache has been invalidated so that they can call findExtensions again to get an updated set of extensions?
There was a problem hiding this comment.
If people want to watch the file-system for changes, then I think it makes sense to build it in here.
You'd need to watch .dart_tool/package_config.json and config.json for every mutable extension.
I imagine that devtools will load extensions when a debugging instance is launched. If I do dart pub get after launching my app, can I expect hotreloading of all dependencies to work?
I don't mind adding some watch logic.
There was a problem hiding this comment.
Adding some watch logic would be great. In the short term, we can probably get by with having a way for the user to manually refresh the list of extensions from the extensions settings menu and by refreshing the list of available extensions on each hot reload or restart event. This of course would only be possible for Flutter. If there was a running dart CLI app that DevTools was connected to, the app would need to be killed, restarted, and reconnected to DevTools to pick up any changes that happened from running dart pub get.
I imagine that devtools will load extensions when a debugging instance is launched. If I do dart pub get after launching my app, can I expect hotreloading of all dependencies to work?
@christopherfujino do you know if all the dependencies are reloaded on hot reload? If we can expect this to work, DevTools can automatically refresh the list of extensions on hot reload / restart.
pkgs/extension_discovery/README.md
Outdated
| // Find extensions for the "hello_world" package. | ||
| // WARNING: This only works when running in JIT-mode, if running in AOT-mode | ||
| // you must supply the `packageConfig` argument, and have a local | ||
| // `.dart_tool/package_config.json` and `$PUB_CACHE`. | ||
| final extensions = await findExtensions('hello_world'); |
There was a problem hiding this comment.
can you provide an example of what this function call would look like in AOT mode when the packageConfig is specified?
There was a problem hiding this comment.
I'm not sure it easy to make a good example for that.
It's kind of like: findExtensions('hello_world', packageConfig: Uri.parse('/home/my_user/workspace/myapp/.dart_tool/package_config.json'));
Where /home/my_user/workspace/myapp is the place I did my source checkout and ran dart pub get.
So in the context of devtools, you'd probably run findExtensions('hello_world', packageConfig: Uri.parse('path/to/the/users/workspace/.dart_tool/package_config.json')).
Where path/to/the/users/workspace/ is the workspace/app for which the user is running devtools.
There was a problem hiding this comment.
This was mostly an attempt at pointing to the "Runtime limitations" section.
| When writing an extension it is strongly encouraged to have a dependency | ||
| constraint on the package being extended. This ensures that the extending |
There was a problem hiding this comment.
just noting that this will not apply for DevTools extensions since we are not shipped as a pub package. Not sure if there are other "non-package" use cases for extensions, or if DevTools is the exception.
There was a problem hiding this comment.
It's recommendation because it really depends on the use-case.
In many cases if you're writing an extension to a package, then you're implementing an interface from the package, so then it's also natural to have a dependency on it.
If you wanted to have to ability to do, you could ask devtools extension packages to have a dependency on devtools_ext and then bump that package when you break the extension interface.
But it's also fair that you just detect that an extension isn't compatible and thus shouldn't be loaded.
In you use-case, not loading the extension is probably better than having a version conflict.
No issue w/ hosting it here, but we will want to start w/ an issue in order to nail down things like who's the primary maintainer, what team is backing them up, what the initial SLO will be, the anticipated final SLO, ... go/create-dart-package-bug. |
| If your deployment target a compiled Flutter application or AOT-compiled | ||
| executable, then you will have to create some code/asset-generation. |
There was a problem hiding this comment.
one caveat is that a profile mode flutter app is AOT compiled but findExtensions would still work if given the location of this app's package_config.json
There was a problem hiding this comment.
Yes, I just think it's important to point out that if you are writing a Flutter app, you can't really use findExtensions and have it magically working on your phone.
Because obviously the package_config.json and all the source code for the dependencies installed in pub-cache, aren't available on your phone, at-least not without some serious hacks.
There was a problem hiding this comment.
I see. I think most of my comments are because I'm reading from the perspective of having another tool (DevTools, which is running on the user's local machine) call findExtensions on a separate app (also running on the user's machine), and the warnings you are heading are when an app would be trying to call findExtensions itself.
|
Left some comments. Overall, looks great! |
Filed b/292190799 |
|
Note to self:
|
Co-authored-by: Christopher Fujino <[email protected]>
| dev_dependencies: | ||
| dart_flutter_team_lints: ^1.0.0 | ||
| lints: ^2.0.0 | ||
| test: ^1.21.0 | ||
| test_descriptor: ^2.0.1 | ||
|
|
||
| environment: | ||
| sdk: ^3.0.0 |
There was a problem hiding this comment.
do all these dependencies already exist in the Dart SDK? or would they be acceptable to bring in? package:extension_discovery will be depended upon by package:devtools_shared, which is a dep of package:dds and therefore the Dart SDK.
There was a problem hiding this comment.
I see lints_rev, test_rev, and test_descriptor_rev in DEPS, but I do not see dart_flutter_team_lints.
There was a problem hiding this comment.
It's in there :) It comes in as part of the dart-lang/ecosystem repo dep.
| description: >- | ||
| Discovery of packages that provide extensions for your package. | ||
| version: 1.0.0-wip | ||
| publish_to: none |
There was a problem hiding this comment.
should this be removed before landing?
There was a problem hiding this comment.
A couple questions around publishing and more importantly, about dependencies since a DEP for package:extension_discovery will need to be added to the Dart SDK in order for DevTools to use this package (https://github.com/dart-lang/sdk/wiki/Adding-and-Updating-Dependencies).
I checked this code out though and verified that package:extension_discovery is WAI for the DevTools extensions use case: flutter/devtools#6139.
LGTM overall.
|
Merged, published: https://pub.dev/packages/extension_discovery and transferred to |
|
@jonasfj we will need to also add this package to the SDK DEPS (https://github.com/dart-lang/sdk/wiki/Adding-and-Updating-Dependencies) and roll into google3. Is that something you can do? |
|
Actually, the tools repo is already in DEPS, so we'll just need to bump the hash. That PR is here: https://dart-review.googlesource.com/c/sdk/+/317804. However, we will still need to setup a google3 mirror for this package. |
Establishing a convention for how allow other packages to provide extensions for your package.
And providing a
findExtensionsmethod that searches all packages inpackage_config.jsonforextension/<targetPackage>/config.json.Also caching detected extensions in
.dart_tool/extension_discovery/<targetPackage>.jsonand comparing with timestamps ofpackage_config.jsonandconfig.json(for relative path-dependencies, which are considered mutable).I'm open to bikeshedding:
extension/<targetPackage>/config.jsonconfig.json(it's a bit slower and more error-prone, but probably fine)But:
extension/seems fitting (none of existing folders in package layout convention seems fitting).@kenzieschmoll, thoughts?
@devoncarew, wdyt about this package living here? I think that means it'll be pulled in to the Dart SDK via DEPS.
I think it's mostly useful for development tools as we have no code-generation hooks that can easily leverage this convention (yet, who knows someday, maybe).