Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1307183002:
Add access to the isolate package-root/package-map in Platform. (Closed)
|
Created:
5 years, 4 months ago by Lasse Reichstein Nielsen Modified:
5 years, 2 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org, sethladd Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd access to the isolate package-root/package-map in Platform.
BUG= http://dartbug.com/24022
Patch Set 1 #
Total comments: 26
Patch Set 2 : replace packageRoot getter, remove C++ imp. #
Messages
Total messages: 22 (3 generated)
lrn@google.com changed reviewers: + floitsch@google.com, iposva@google.com, sgjesse@google.com
WDYT?
LGTM. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:183: * If there is no --package-root flag, then the empty string is returned. should that one also return null in case of a package-map? https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:197: * Returns the package mapping of the current isolate, if any. Should we mention that the package-mapping (as well as root) is the one that is currently active. For compiled code it might differ from where the sources came from? (We need an "advanced" section in dartdocs). https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:202: * that package are resolved against, or to `null`, if the package name is no need to specify the `null` case. That's the normal behavior of maps.
nweiz@google.com changed reviewers: + nweiz@google.com
LGTM! https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:192: * URIs are resolved against. It would be good to explicitly mention how this differs from [packageRoot]. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:199: * If the current isolate is using [packageRoot], this getter returns `null`. Shouldn't this be [packageRootUri]? Presumably this will also be `null` if the main isolate uses a package root because the implicit detection of .packages failed, even if --package-root hasn't been passed.
More in-depth review forthcoming. -Ivan https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:206: * URIs. Should return null if the isolate is not using a package map. Otherwise you cannot distinguish between: Found a .packages file (or was passed an empty map through the API) from package map not found/passed/
https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:183: * If there is no --package-root flag, then the empty string is returned. Not touching existing functionality. That could be a breaking change. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:192: * URIs are resolved against. On 2015/08/25 01:17:05, nweiz wrote: > It would be good to explicitly mention how this differs from [packageRoot]. Acknowledged. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:197: * Returns the package mapping of the current isolate, if any. On 2015/08/24 15:09:45, floitsch wrote: > Should we mention that the package-mapping (as well as root) is the one that is > currently active. For compiled code it might differ from where the sources came > from? > > (We need an "advanced" section in dartdocs). Acknowledged. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:199: * If the current isolate is using [packageRoot], this getter returns `null`. Good catch. I'm not sure if a default package root for an HTTP base would not show up here. I'll test that. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:202: * that package are resolved against, or to `null`, if the package name is True. Should just say that it only has keys for a finite set of recognized package names. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:206: * URIs. I chose to do this instead because it's more useful. This is not intended as a way to debug your package resolution detection, just to provide what is currently being used. I assumed that not having any resolution is indistinguishable from have an empty map (no package name recognized), and it gives the user one less case to worry about - one of the two getters always returns non-null.
lgtm
kevmoo@google.com changed reviewers: + kevmoo@google.com
DBC: include a corresponding update to CHANGELOG.md, please :-)
This does not Look Good in the current state as the accessors are all synchronous for data that is resolved asynchronously. -Ivan https://codereview.chromium.org/1307183002/diff/1/runtime/bin/platform.cc File runtime/bin/platform.cc (right): https://codereview.chromium.org/1307183002/diff/1/runtime/bin/platform.cc#new... runtime/bin/platform.cc:88: void FUNCTION_NAME(Platform_PackageResolution)(Dart_NativeArguments args) { There should be no need to rely on C++ code for this feature. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:183: * If there is no --package-root flag, then the empty string is returned. On 2015/08/25 06:23:00, Lasse Reichstein Nielsen wrote: > Not touching existing functionality. That could be a breaking change. To be honest we should consider making this breaking change and even consider completely removing this value as it is mostly useless, and we know that currently it can also be entirely wrong in the current implementation. This last fact hints to me that we have very few, if any uses of this accessor. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:190: * If the isolate is using a [packageMap], this getter returns `null`, This is not entirely correct: An isolate might not be using a packageMap and not be using a packageRoot at the same time. For example when loaded from a snapshot. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:192: * URIs are resolved against. On 2015/08/25 06:23:00, Lasse Reichstein Nielsen wrote: > On 2015/08/25 01:17:05, nweiz wrote: > > It would be good to explicitly mention how this differs from [packageRoot]. > > Acknowledged. If we decide to remove packageRoot, then we do not need to document its difference to the non-existing accessor. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:194: static Uri get packageRootUri => _Platform.packageRootUri; All the new getters here need to return Futures. The package file resolution is asynchronous and is only triggered on first access. In the case of launching from a snapshot or when no packages are being loaded for the main entry point (e.g. deferred loading) we will reach user code which can call these accessors before their data has been resolved. The lazy resolution of the packages location is mandatory to not negatively impact launches which do not rely on packages to be loaded, which includes all deployment scenarios from snapshots. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:206: * URIs. On 2015/08/25 06:23:00, Lasse Reichstein Nielsen wrote: > I chose to do this instead because it's more useful. > This is not intended as a way to debug your package resolution detection, just > to provide what is currently being used. > I assumed that not having any resolution is indistinguishable from have an empty > map (no package name recognized), and it gives the user one less case to worry > about - one of the two getters always returns non-null. Isn't that exactly what the null-aware operators would be for? If you don't care about the fact that a neither a packageRoot or a packageMap are in use just do var mapping = packageMap?[package_name]; https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform_impl.dart File sdk/lib/io/platform_impl.dart (right): https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform_impl.da... sdk/lib/io/platform_impl.dart:46: if (_resolution is Uri) return _resolution; I'll implement these without going through native code. How about throwing unsupported errors for now?
https://codereview.chromium.org/1307183002/diff/1/runtime/bin/platform.cc File runtime/bin/platform.cc (right): https://codereview.chromium.org/1307183002/diff/1/runtime/bin/platform.cc#new... runtime/bin/platform.cc:88: void FUNCTION_NAME(Platform_PackageResolution)(Dart_NativeArguments args) { Excellent. It did seem a bit overengineered to go through C++ to access a Dart feature. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:183: * If there is no --package-root flag, then the empty string is returned. I'm all for breaking, in which case packageRoot should be the name for the URI, not packageRootUri. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:190: * If the isolate is using a [packageMap], this getter returns `null`, Again, I'm trying to keep things simpler by saying that an isolate always use one of the two options (with "nothing" being equivalent to an empty map). https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:192: * URIs are resolved against. On 2015/08/26 04:29:36, Ivan Posva wrote: > On 2015/08/25 06:23:00, Lasse Reichstein Nielsen wrote: > > On 2015/08/25 01:17:05, nweiz wrote: > > > It would be good to explicitly mention how this differs from [packageRoot]. > > > > Acknowledged. > > If we decide to remove packageRoot, then we do not need to document its > difference to the non-existing accessor. Acknowledged. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:194: static Uri get packageRootUri => _Platform.packageRootUri; Would it be possible to be blocking instead of async? I wouldn't mind if he first access here blocked until the value was available. The fact that the VM handles things lazily is an implementation detail that we don't need to expose to users. (I did notice some bug reports that looked like a lazy initialization being done a little too late). https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:206: * URIs. That would be an argument if ?[] was actually in the language. We only have a half-baked null-aware specification where ?.foo works, but neither ?<operator> nor ?(). I know it's harder to parse, but it's still feels like half a solution. It's not just to make it easier to write - it's to make it easier to understand for the user (and therefore use correctly). If there are only two cases: use package-root/use package-map, then it's impossible to forget he null case. https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform_impl.dart File sdk/lib/io/platform_impl.dart (right): https://codereview.chromium.org/1307183002/diff/1/sdk/lib/io/platform_impl.da... sdk/lib/io/platform_impl.dart:46: if (_resolution is Uri) return _resolution; That's fine. I'll just add the interface for now.
Making these APIs asynchronous would be a serious problem for package ecosystem users. Asynchrony is contagious, in the sense that as soon as one API becomes asynchronous everything above it has to be as well. Even with async/await, this will cause serious API design issues. In practice, any code that contains "package:" imports will have already loaded the package map and have it (synchronously) available in its isolate. This means that effectively *all* code that would want access to the packages map will be able to access it synchronously.
On 2015/08/26 19:21:58, nweiz wrote: > Making these APIs asynchronous would be a serious problem for package ecosystem > users. Asynchrony is contagious, in the sense that as soon as one API becomes > asynchronous everything above it has to be as well. Even with async/await, this > will cause serious API design issues. While I agree with you that in principle synchronous APIs often are easier to deal with, we have built an asynchronous system. Everything related to external interaction is asynchronous and so is loading (e.g. deferred loading). > In practice, any code that contains "package:" imports will have already loaded > the package map and have it (synchronously) available in its isolate. This means > that effectively *all* code that would want access to the packages map will be > able to access it synchronously. You have too many failed assumptions in your above statement: - A program loaded from a snapshot has not touched the package resolution code. The reason we enabled the searching of the relevant .packages file and packages/ directory was due to the fact that deployed applications (running from a snapshot) will not have to touch any external resources to enable fast startup. - A program which only lazily loads packages through deferred loading will not have the package resolution pre loaded. - A program which only uses packages to access external resources will not have the package resolution pre loaded when starting main. In short, whether we like it or not we do have an asynchronous system and trying to force a single API being synchronous does not fit. -Ivan
On 2015/08/27 04:28:23, Ivan Posva wrote: > On 2015/08/26 19:21:58, nweiz wrote: > > Making these APIs asynchronous would be a serious problem for package > ecosystem > > users. Asynchrony is contagious, in the sense that as soon as one API becomes > > asynchronous everything above it has to be as well. Even with async/await, > this > > will cause serious API design issues. > > While I agree with you that in principle synchronous APIs often are easier to > deal with, we have built an asynchronous system. Everything related to external > interaction is asynchronous and so is loading (e.g. deferred loading). Anything that *loads* a file is asynchronous, but imagine if const String.fromEnvironment had to be async - it would be useless. This is, what appears to be, a simple property being read, one that the user can reasonably assume the system already knows. It's just the VM has made an optimization where it only fetches that information lazily when it's needed. That's ok if it's transparent. If we make these getters async to match the optimization, then it's no longer transparent. > > > In practice, any code that contains "package:" imports will have already > loaded > > the package map and have it (synchronously) available in its isolate. This > means > > that effectively *all* code that would want access to the packages map will be > > able to access it synchronously. > > You have too many failed assumptions in your above statement: > - A program loaded from a snapshot has not touched the package resolution code. > The reason we enabled the searching of the relevant .packages file and packages/ > directory was due to the fact that deployed applications (running from a > snapshot) will not have to touch any external resources to enable fast startup. > - A program which only lazily loads packages through deferred loading will not > have the package resolution pre loaded. > - A program which only uses packages to access external resources will not have > the package resolution pre loaded when starting main. > > In short, whether we like it or not we do have an asynchronous system and trying > to force a single API being synchronous does not fit. I don't think it's obvious from the outside that the system is asynchronous, it is, as I write, an implementation detail that I don't want to leak in the API if we can avoid it. The VM gets away with being lazy because all other APIs that use package resolution are for fetching files, which we do make asynchronous, but I expect the user's mental model is that the system *has* a way to resolve package URIs, it's not something it needs to find. Going against the mental model is dangerous. I am willing to block the entire isolate while fetching the package resolution information. That may be a long time if the .packages needs to be fetched over HTTP. That's not unprecedented, we have lots of Sync functions in dart:io. We could have packageMapSync and packageRootSync getters, I'd just prefer them to be the default.
On 2015/08/27 05:16:49, Lasse Reichstein Nielsen wrote: > On 2015/08/27 04:28:23, Ivan Posva wrote: > > On 2015/08/26 19:21:58, nweiz wrote: > > > Making these APIs asynchronous would be a serious problem for package > > ecosystem > > > users. Asynchrony is contagious, in the sense that as soon as one API > becomes > > > asynchronous everything above it has to be as well. Even with async/await, > > this > > > will cause serious API design issues. > > > > While I agree with you that in principle synchronous APIs often are easier to > > deal with, we have built an asynchronous system. Everything related to > external > > interaction is asynchronous and so is loading (e.g. deferred loading). > > Anything that *loads* a file is asynchronous, but imagine if const > String.fromEnvironment had to be async - it would be useless. > > This is, what appears to be, a simple property being read, one that the user > can reasonably assume the system already knows. It's just the VM has made > an optimization where it only fetches that information lazily when it's needed. > That's ok if it's transparent. If we make these getters async to match the > optimization, then it's no longer transparent. > > > > > > > In practice, any code that contains "package:" imports will have already > > loaded > > > the package map and have it (synchronously) available in its isolate. This > > means > > > that effectively *all* code that would want access to the packages map will > be > > > able to access it synchronously. > > > > You have too many failed assumptions in your above statement: > > - A program loaded from a snapshot has not touched the package resolution > code. > > The reason we enabled the searching of the relevant .packages file and > packages/ > > directory was due to the fact that deployed applications (running from a > > snapshot) will not have to touch any external resources to enable fast > startup. > > - A program which only lazily loads packages through deferred loading will not > > have the package resolution pre loaded. > > - A program which only uses packages to access external resources will not > have > > the package resolution pre loaded when starting main. > > > > In short, whether we like it or not we do have an asynchronous system and > trying > > to force a single API being synchronous does not fit. > > I don't think it's obvious from the outside that the system is asynchronous, it > is, > as I write, an implementation detail that I don't want to leak in the API if we > can > avoid it. The VM gets away with being lazy because all other APIs that use > package > resolution are for fetching files, which we do make asynchronous, but I expect > the > user's mental model is that the system *has* a way to resolve package URIs, it's > not something it needs to find. Going against the mental model is dangerous. > > I am willing to block the entire isolate while fetching the package resolution > information. That may be a long time if the .packages needs to be fetched over > HTTP. That's not unprecedented, we have lots of Sync functions in dart:io. > We could have packageMapSync and packageRootSync getters, I'd just prefer them > to be > the default. The fact is that to get at the package map we need to communicate to a different isolate -> it is asynchronous. Sorry, -Ivan
On 2015/08/27 05:21:09, Ivan Posva wrote: > On 2015/08/27 05:16:49, Lasse Reichstein Nielsen wrote: > > On 2015/08/27 04:28:23, Ivan Posva wrote: > > > On 2015/08/26 19:21:58, nweiz wrote: > > > > Making these APIs asynchronous would be a serious problem for package > > > ecosystem > > > > users. Asynchrony is contagious, in the sense that as soon as one API > > becomes > > > > asynchronous everything above it has to be as well. Even with async/await, > > > this > > > > will cause serious API design issues. > > > > > > While I agree with you that in principle synchronous APIs often are easier > to > > > deal with, we have built an asynchronous system. Everything related to > > external > > > interaction is asynchronous and so is loading (e.g. deferred loading). > > > > Anything that *loads* a file is asynchronous, but imagine if const > > String.fromEnvironment had to be async - it would be useless. > > > > This is, what appears to be, a simple property being read, one that the user > > can reasonably assume the system already knows. It's just the VM has made > > an optimization where it only fetches that information lazily when it's > needed. > > That's ok if it's transparent. If we make these getters async to match the > > optimization, then it's no longer transparent. > > > > > > > > > > > In practice, any code that contains "package:" imports will have already > > > loaded > > > > the package map and have it (synchronously) available in its isolate. This > > > means > > > > that effectively *all* code that would want access to the packages map > will > > be > > > > able to access it synchronously. > > > > > > You have too many failed assumptions in your above statement: > > > - A program loaded from a snapshot has not touched the package resolution > > code. > > > The reason we enabled the searching of the relevant .packages file and > > packages/ > > > directory was due to the fact that deployed applications (running from a > > > snapshot) will not have to touch any external resources to enable fast > > startup. > > > - A program which only lazily loads packages through deferred loading will > not > > > have the package resolution pre loaded. > > > - A program which only uses packages to access external resources will not > > have > > > the package resolution pre loaded when starting main. > > > > > > In short, whether we like it or not we do have an asynchronous system and > > trying > > > to force a single API being synchronous does not fit. > > > > I don't think it's obvious from the outside that the system is asynchronous, > it > > is, > > as I write, an implementation detail that I don't want to leak in the API if > we > > can > > avoid it. The VM gets away with being lazy because all other APIs that use > > package > > resolution are for fetching files, which we do make asynchronous, but I expect > > the > > user's mental model is that the system *has* a way to resolve package URIs, > it's > > not something it needs to find. Going against the mental model is dangerous. > > > > I am willing to block the entire isolate while fetching the package resolution > > information. That may be a long time if the .packages needs to be fetched over > > HTTP. That's not unprecedented, we have lots of Sync functions in dart:io. > > We could have packageMapSync and packageRootSync getters, I'd just prefer them > > to be > > the default. > > The fact is that to get at the package map we need to communicate to a different > isolate -> it is asynchronous. > > Sorry, > -Ivan P.S. This is not even start taking into account how to make a synchronous HTTP call in Dart to get at the .packages file for a http hosted program.
On 2015/08/27 05:31:50, Ivan Posva wrote: > On 2015/08/27 05:21:09, Ivan Posva wrote: > > On 2015/08/27 05:16:49, Lasse Reichstein Nielsen wrote: > > > On 2015/08/27 04:28:23, Ivan Posva wrote: > > > > On 2015/08/26 19:21:58, nweiz wrote: > > > > > Making these APIs asynchronous would be a serious problem for package > > > > ecosystem > > > > > users. Asynchrony is contagious, in the sense that as soon as one API > > > becomes > > > > > asynchronous everything above it has to be as well. Even with > async/await, > > > > this > > > > > will cause serious API design issues. > > > > > > > > While I agree with you that in principle synchronous APIs often are easier > > to > > > > deal with, we have built an asynchronous system. Everything related to > > > external > > > > interaction is asynchronous and so is loading (e.g. deferred loading). > > > > > > Anything that *loads* a file is asynchronous, but imagine if const > > > String.fromEnvironment had to be async - it would be useless. > > > > > > This is, what appears to be, a simple property being read, one that the user > > > > can reasonably assume the system already knows. It's just the VM has made > > > an optimization where it only fetches that information lazily when it's > > needed. > > > That's ok if it's transparent. If we make these getters async to match the > > > optimization, then it's no longer transparent. > > > > > > > > > > > > > > > In practice, any code that contains "package:" imports will have already > > > > loaded > > > > > the package map and have it (synchronously) available in its isolate. > This > > > > means > > > > > that effectively *all* code that would want access to the packages map > > will > > > be > > > > > able to access it synchronously. > > > > > > > > You have too many failed assumptions in your above statement: > > > > - A program loaded from a snapshot has not touched the package resolution > > > code. > > > > The reason we enabled the searching of the relevant .packages file and > > > packages/ > > > > directory was due to the fact that deployed applications (running from a > > > > snapshot) will not have to touch any external resources to enable fast > > > startup. > > > > - A program which only lazily loads packages through deferred loading will > > not > > > > have the package resolution pre loaded. > > > > - A program which only uses packages to access external resources will not > > > have > > > > the package resolution pre loaded when starting main. > > > > > > > > In short, whether we like it or not we do have an asynchronous system and > > > trying > > > > to force a single API being synchronous does not fit. > > > > > > I don't think it's obvious from the outside that the system is asynchronous, > > it > > > is, > > > as I write, an implementation detail that I don't want to leak in the API if > > we > > > can > > > avoid it. The VM gets away with being lazy because all other APIs that use > > > package > > > resolution are for fetching files, which we do make asynchronous, but I > expect > > > the > > > user's mental model is that the system *has* a way to resolve package URIs, > > it's > > > not something it needs to find. Going against the mental model is dangerous. > > > > > > I am willing to block the entire isolate while fetching the package > resolution > > > information. That may be a long time if the .packages needs to be fetched > over > > > HTTP. That's not unprecedented, we have lots of Sync functions in dart:io. > > > We could have packageMapSync and packageRootSync getters, I'd just prefer > them > > > to be > > > the default. > > > > The fact is that to get at the package map we need to communicate to a > different > > isolate -> it is asynchronous. > > > > Sorry, > > -Ivan > > P.S. This is not even start taking into account how to make a synchronous HTTP > call in Dart to get at the .packages file for a http hosted program. I'm sure it's doable, the question is how much inconvenience it would be (just block the current thread and let another thread do the async operation, then come back when the answer is ready). Since you are already using another isolate to do the work, it's a matter of running a second thread with access to modify the current isolate, while the isolate execution thread is blocked. /L
As someone who has written a lot of command-line Dart apps, and someone who has had to write gnarly logic to find package contents at runtime in all the various ways a Dart app can be installed and run, I can say that Futures are infectious and a pain to deal with. **However**, I can also say that doing anything sufficiently interesting with our platform means the developer will need to use a Future somehow and someway. Even if the call to packageMap was synchronous, I'm almost certainly going to deal with async APIs (and thus, Future) while doing anything with the packageMap results. If only because package: URIs and package: contents can and will use the http: scheme. So any general logic dealing with package: contents and the packageMap already needs to deal with Futures. Therefore, because we can't avoid touching a Future when handling package: , I don't see the issue of making packageMap an async call. I'm writing this as a user. This little API unblocks a significant improvement to our platform: the elimination of symlinks. That's a win that's too big for us to ignore.
> P.S. This is not even start taking into account how to make a synchronous HTTP > call in Dart to get at the .packages file for a http hosted program. The VM already does synchronous HTTP requests: https://github.com/dart-lang/sdk/issues/12617 On 2015/08/27 16:46:29, sethladd wrote: > As someone who has written a lot of command-line Dart apps, and someone who has > had to write gnarly logic to find package contents at runtime in all the various > ways a Dart app can be installed and run, I can say that Futures are infectious > and a pain to deal with. > > **However**, I can also say that doing anything sufficiently interesting with > our platform means the developer will need to use a Future somehow and someway. > Even if the call to packageMap was synchronous, I'm almost certainly going to > deal with async APIs (and thus, Future) while doing anything with the packageMap > results. If only because package: URIs and package: contents can and will use > the http: scheme. So any general logic dealing with package: contents and the > packageMap already needs to deal with Futures. > > Therefore, because we can't avoid touching a Future when handling package: , I > don't see the issue of making packageMap an async call. I'm writing this as a > user. I've also written a bunch of command-line Dart apps, and maintained one of the biggest ones for years, and I disagree. Although it's true that it's often impossible to avoid async code in some places—in particular isolates, HTTP, and complex Process code—it's also true that everywhere it *can* be avoided makes the code look nicer and run faster, and it allows us to expose APIs that would be straight-up impossible with async code. Two examples. First, years ago, we moved from async to sync file IO in pub. In addition to slightly-but-measurably improving performance, it made the code vastly more readable and allowed us to do a bunch of refactors, making things that were methods (to communicate that they were asynchronous) into properties. We also ripped out a bunch of code that defended against potential race conditions. Second, just recently, we changed a widely-used method on the Source class from async to sync. Even though we had already been using async/await in many places by that point, we were able to make a whole cascade of improvements throughout the codebase. All the code got more terse and understandable, and we were able to do things like use getters directly rather than storing their values in an instance field or use higher-order Iterable methods that just don't work with asynchronous components. My point is that even today, asynchrony has a high cost for application development, and that *anywhere* that cost can be avoided is a benefit. Even if other parts of the system are asynchronous, every bit that can be kept synchronous—especially low-level APIs—is a win. If we can make Platform.packageMap and Platform.packagesRoot synchronous at the cost of potentially blocking a narrow set of code that doesn't already have it loaded, that's a trade I'm thrilled to make. > This little API unblocks a significant improvement to our platform: the > elimination of symlinks. That's a win that's too big for us to ignore. I don't think anyone is arguing against this API. But just because it's urgent doesn't make the design any less important.
On 2015/08/28 01:06:28, nweiz wrote: > > P.S. This is not even start taking into account how to make a synchronous HTTP > > call in Dart to get at the .packages file for a http hosted program. > > The VM already does synchronous HTTP requests: > https://github.com/dart-lang/sdk/issues/12617 > > On 2015/08/27 16:46:29, sethladd wrote: > > As someone who has written a lot of command-line Dart apps, and someone who > has > > had to write gnarly logic to find package contents at runtime in all the > various > > ways a Dart app can be installed and run, I can say that Futures are > infectious > > and a pain to deal with. > > > > **However**, I can also say that doing anything sufficiently interesting with > > our platform means the developer will need to use a Future somehow and > someway. > > Even if the call to packageMap was synchronous, I'm almost certainly going to > > deal with async APIs (and thus, Future) while doing anything with the > packageMap > > results. If only because package: URIs and package: contents can and will use > > the http: scheme. So any general logic dealing with package: contents and the > > packageMap already needs to deal with Futures. > > > > Therefore, because we can't avoid touching a Future when handling package: , I > > don't see the issue of making packageMap an async call. I'm writing this as a > > user. > > I've also written a bunch of command-line Dart apps, and maintained one of the > biggest ones for years, and I disagree. Although it's true that it's often > impossible to avoid async code in some places—in particular isolates, HTTP, and > complex Process code—it's also true that everywhere it *can* be avoided makes > the code look nicer and run faster, and it allows us to expose APIs that would > be straight-up impossible with async code. > > Two examples. First, years ago, we moved from async to sync file IO in pub. In > addition to slightly-but-measurably improving performance, it made the code > vastly more readable and allowed us to do a bunch of refactors, making things > that were methods (to communicate that they were asynchronous) into properties. > We also ripped out a bunch of code that defended against potential race > conditions. > > Second, just recently, we changed a widely-used method on the Source class from > async to sync. Even though we had already been using async/await in many places > by that point, we were able to make a whole cascade of improvements throughout > the codebase. All the code got more terse and understandable, and we were able > to do things like use getters directly rather than storing their values in an > instance field or use higher-order Iterable methods that just don't work with > asynchronous components. > > My point is that even today, asynchrony has a high cost for application > development, and that *anywhere* that cost can be avoided is a benefit. Even if > other parts of the system are asynchronous, every bit that can be kept > synchronous—especially low-level APIs—is a win. If we can make > Platform.packageMap and Platform.packagesRoot synchronous at the cost of > potentially blocking a narrow set of code that doesn't already have it loaded, > that's a trade I'm thrilled to make. > > > This little API unblocks a significant improvement to our platform: the > > elimination of symlinks. That's a win that's too big for us to ignore. > > I don't think anyone is arguing against this API. But just because it's urgent > doesn't make the design any less important. This is dead, right?
It is dead, Ivan took over the implementation. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
