Skip to content

Move Platform (or similar) to dart:core #35969

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

Closed
yjbanov opened this issue Feb 17, 2019 · 57 comments
Closed

Move Platform (or similar) to dart:core #35969

yjbanov opened this issue Feb 17, 2019 · 57 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue customer-flutter library-core
Milestone

Comments

@yjbanov
Copy link

yjbanov commented Feb 17, 2019

While adding Web support to Flutter we'd like to minimize the amount of disruption for Flutter developers, and maximize their code portability. As part of that, we'd like to make as much of the standard library API as possible available to the Web compilation target.

Flutter uses Platform from dart:io to detect current OS. This function should be implementable on the Web via Window.navigator.

/cc @vsmenon @kevmoo

@sortie
Copy link
Contributor

sortie commented Feb 18, 2019

Are there any other parts of dart:io that you would like to use on the web?

@lrhn We talked a bit about the the Platform class before, any thoughts?

@yjbanov
Copy link
Author

yjbanov commented Feb 18, 2019

I was going to create issues for other parts separately, as I discover them, but to answer your question, I think we may also need the subset of the File/Directory API that deals with manipulating file paths (example).

@lrhn
Copy link
Member

lrhn commented Feb 26, 2019

I would very much recommend not making dart:io available on platforms that cannot support the majority of the API. The dart:io library is a hodge-podge of Posix OS functionality (filesystems, network sockets, HTTP client and server, etc.). Most of this makes absolutely no sense in a browser, and even if some of the concepts do, the chosen APIs will be incompatible in subtle ways.

It's far safer to just say no to dart:io if it isn't actually working. We have conditional imports which check if dart:io is available, and which might take a branch that tries to use dart:io's HttpClient over dart:html's HttpRequest, if it thinks dart:io is there. If you can import dart:io, but it doesn't work, you break this feature detection. (We may actually allow importing dart:io on the web because it allowed code predating conditional imports to compile, but we should move away from that hack now that we can). If the platform says that dart:io is available, the users should be able to import and use it, with its complete API, not just some very small subset. If it's not available, the user should not import it. The middle ground is very, very slippery for users.

If there is something non-platform specific or generalizable in dart:io, I'd recommend creating a new dart:something library with just that functionality (the name would depend on what it is). I don't expect there to be much, but dart:_http is shared between dart:io and dart:html, it's just not exposed by its own.

If the only functionality you need is the OS name, which is just one string, then I'd just make const String.fromEnvironment("dart.platform.os") always return the identifying string, and not bother with a library.

The isLinux, isMacOS, etc. getters are just simple wrappers that can be put in any package. They're literally:

  static String get operatingSystem => _operatingSystem;
  static final bool isLinux = (_operatingSystem == "linux");

Those booleans don't need to be in the platform libraries, and indeed it makes it harder to port Dart to a new OS when you have to add an isGoodieOS getter to the SDK libraries to properly support it. Operating system is not a closed enum, hardcoding it in source that users can't easily update is a bad idea.

Also, "the subset of the File/Directory API that deals with manipulating file paths" isn't that much, it's basically the one static function FileSystemEntity.parentOf, together with Platform.pathSeparator (which is again a simple string, so you could just do use String.fromEnvironment("dart.platform.pathSeparator"), and the dirname function from package:path does exactly the same thing as parentOf, so you don't actually need it.

@yjbanov
Copy link
Author

yjbanov commented Mar 4, 2019

@lrhn I think your solution would be fine if we had no existing Dart code in the wild. However, currently in Flutter dart:io is the only way to get platform information and existing code uses it. I don't see a practical way we can remove Platform from dart:io. Nor do I see a way to remove dart:io from Flutter. So if Platform is not supported by DDC/dart2js, then we will end up with the following:

  • Duplicate shared Platform functionality in another dart:* library.
  • Use conditional imports to route between dart:io and the other thing.
  • Not achieve the "no disruption" goal.

While I agree that having dart:io be only partially supported on the Web is not a clean solution, I'm not sure the complexity of duplicate functionality + conditional imports + disruption is any cleaner?

@lrhn
Copy link
Member

lrhn commented Mar 6, 2019

I would move the shared Platform functionality to another dart:* library.
Then unconditionally import that into dart:io and use it for the dart:io Platform class.
That keeps dart:io's Platform available, while also providing a new and direct access to the functionality.

Client libraries can directly and unconditionally import the other dart:* library instead of dart:io if all they need is platform information.
Client libraries that use any other dart:io functionality would still not work on non-IO-supporting platforms anyway.

It will require a migration for code that uses dart:io only for platform flags, in order for them to be compilable to platforms without dart:io. That was always the case, nothing new here. You can't depend on something which isn't there. Claiming it's there means that someone might use it, and that's a run-time failure instead of a compile-time failure.

No matter what we do, making dart:io available on a platform that doesn't support a majority of the functionality is actively harmful, and we should not do that. Making the right size of puzzle pieces and making them fit together is a design task that we can iterate on. That all comes down to which functionality we are talking about. How much is it? How general is it?
If the shared functionality is equivalent to the Platform class declaration, we can just move that in its entirety, and make dart:io export the declaration.

Just creating a fake dart:io is an anti-goal, even in the short run.

@kevmoo
Copy link
Member

kevmoo commented Mar 6, 2019 via email

@yjbanov
Copy link
Author

yjbanov commented Mar 7, 2019

@lrhn

Ok, I think I'm sold on the idea that if we give access to a dart:* library then developers should be able to use most of it and not discover issues after they compile their code. Moving common functionality into a new dart:* library addresses the duplication issue and removes the need for conditional imports for newly written code. What feels wrong to me is that us adding another compilation target to the Flutter platform immediately requires that we refactor core libraries. It's like adding another compilation target to LLVM and immediately require that C++ refactors its standard library because some subset of it is not supported by the new target.

Perhaps we are looking at the problem from different angles. I think there is middle ground though.

The way I see it is we have Flutter that currently provides access to a bunch of dart:* libraries, and there's tons of Flutter code written in the wild. Now we want to add Web as a sibling to Android and iOS as another compiler option, i.e. flutter build web instead of flutter build apk. Now, I understand the limitations of the Web, and what we are realizing now is that with the Web in the mix we need to rethink what core libraries we should be making available in Flutter and we need to find ergonomic solutions for writing platform-specific code. However, Flutter Web is in a very early stage, and our current goal is to make it work on the Web as well as possible. Once we have that, we can also make it as ergonomic as possible. The two problems seem very much orthogonal to each other, and both require some work (e.g. according to your proposal there needs to be some code migration). So I think that going straight for the cleanest most ergonomic design is not the right choice in terms of sequencing work. It creates a dependency between these two aspect and slows down our progress.

From your angle, it seems the risk is messing up existing Dart ecosystem by introducing dart:io to Web code. If that is so, then I think there is a solution for that:

What if we make dart:io available only to the flutter platform, and not to existing Dart Web code (e.g. AngularDart apps)? This will let us make progress with Flutter Web while in parallel we can plan how to refactor dart:* libraries to make them more ergonomic to use in the presence of platforms that are dart:io-capable and those that are not.

@kevmoo
Copy link
Member

kevmoo commented Mar 7, 2019

Packages will have to do work to support the web anyway. Instead of adding special-case hacks, let's figure out a common package (or dart: lib, I guess). We can create lints/hints to help folks start migrating now.

@yjbanov
Copy link
Author

yjbanov commented Mar 7, 2019

I think it's a little early for a migration. There's a lot of value in learning from the current Flutter ecosystem in a non-disruptive way before we ask people to start adapting their code. This will give us time to design a solution and plan out migration steps. I just want to keep it independent from the goal of launching web support in Flutter.

@lrhn
Copy link
Member

lrhn commented Mar 7, 2019

What if we make dart:io available only to the flutter platform, and not to existing Dart Web code (e.g. AngularDart apps)?

Well, we can't stop you from doing that. The Flutter tools can add any dart:* library to their platform, like they already add dart:flutter. Adding dart:io to web compilation is just not something we'd want to encourage, or make changes in the SDK to enable.

You will risk that configurable imports might choose dart:io over dart:html for some features, and they then fail. And again, you can break protocol and make dart.library.io be false even though dart:io is available, which risks people doing configurable imports to import dart:io if dart.library.flutter.

@yjbanov
Copy link
Author

yjbanov commented Mar 8, 2019

Well, we can't stop you from doing that. The Flutter tools can add any dart:* library to their platform.

I'm not suggesting a mutiny, just trying to find an option that satisfies our constraints. My understanding was that dart2js/DDC implemented some compiler intrinsics around dart:io, so you can import it but the compiler crashes if it detects that you are actually using it. Is that not the case?

Adding it to the Flutter's build of the SDK makes sense. This way we won't be polluting existing Web code or the Dart SDK. It will also temporarily unblock us until we decide on the long-term solution.

@jonahwilliams How straightforward is it to add a custom dart:io implementation to the Flutter's build of the Dart SDK?

You will risk that configurable imports might choose dart:io over dart:html for some features.

As of now whether you use dart:io or dart:html the risk level is the same. We don't yet know how/if dart:html will be available to Flutter apps. We currently expose it but not because we decided that this is the best way to give apps access to the platform but because it was the easiest thing to do. However, of the two, I think the choice of dart:io is better because of backwards-compatibility.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Mar 8, 2019

@jonahwilliams How straightforward is it to add a custom dart:io implementation to the Flutter's build of the Dart SDK?

It's not straightforward at all really. Presumably we could only really support a subset of dart:io, so this would need to be done by supplying our own patch files, building our own dartdevc sdk, and so on. Then there are the code size concerns, et cetera.

Ultimately it would be a lot of duplicated work, and if we're only realistically going to support 1 or 2 types then I'm more in favor of using conditional imports. While there are plenty of existing flutter apps that make heavy use of dart:io, there are no existing flutter apps which compile to the web so I don't see this as breaking. Ultimately a user attempt to port their application to run on the web will need to make some adjustments regardless.

@vsmenon vsmenon added the area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. label Jul 22, 2019
@vsmenon vsmenon changed the title implement dart:io Platform in DDC and dart2js Move Platform (or similar) to dart:core Aug 1, 2019
@vsmenon vsmenon added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core and removed area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. web-dart2js web-dev-compiler library-io labels Aug 1, 2019
@vsmenon vsmenon added this to the Future milestone Aug 1, 2019
@vsmenon
Copy link
Member

vsmenon commented Aug 1, 2019

I think we should go ahead and move Platform from dart:io (and forward) as discussed above. Any objections?

@yjbanov
Copy link
Author

yjbanov commented Aug 5, 2019

@vsmenon Is there a concrete proposal for where exactly to move it?

@kevmoo
Copy link
Member

kevmoo commented Sep 12, 2019

Another option, which should really be the default option as @munificent pointed out, is to not introduce a dart:platform at all, and simply introduce the desired data into dart:html.

We can then still use a package:platform package to provide access to the information which is available on both platforms using conditional imports of libraries using either dart:io or dart:html.

I like this idea!

@yjbanov
Copy link
Author

yjbanov commented Sep 12, 2019

@lrhn

but with different feature selection.

Do you have an idea for what this feature selection would look like? The only thing that comes to mind is this.

@leafpetersen
Copy link
Member

Do you have an idea for what this feature selection would look like? The only thing that comes to mind is this.

I believe that @lrhn is referring to things for which a subset of the full features are available on some platforms. So the File interface is a concrete one which has been brought up - the async APIs might be reasonable to support on the web, but not the sync APIs. So the package would want to offer up only the things which are in the intersection of the features available on the individual platforms.

@yjbanov
Copy link
Author

yjbanov commented Sep 12, 2019

Oh, I totally misread that, sorry. I thought there would be one library that allows dynamically checking which features are supported. But could we support dynamic feature selection? This way we would only need one library that scales to all platforms.

@lrhn
Copy link
Member

lrhn commented Sep 13, 2019

If we have different implementations depending on the supported library, dart:html or dart:io, then that implementation can also add features which only work on one of the platforms, and ways to query the supported feature set.

It's usually a bad idea. Users won't check individual features, they'll just check which platform they are on, and use whatever features they know work on that platform. Or forget to check, and only work on one platform. Or wait for someone else to build an abstraction library on top which only exposes the features that are available everywhere.

We could make the "GeneralFile" class expose the dart:io File object indirectly, allowing people to break the abstraction if they need to, and know what they are doing. Again, that code will only work if you import dart:io, and unless you are also using conditional imports, you won't be needing the general library then.

@mit-mit
Copy link
Member

mit-mit commented Sep 18, 2019

Have we reached agreement here to move forward with the option described in #35969 (comment)?

@yjbanov
Copy link
Author

yjbanov commented Sep 18, 2019

@lrhn

It's usually a bad idea. Users won't check individual features, they'll just check which platform they are on, and use whatever features they know work on that platform. Or forget to check, and only work on one platform. Or wait for someone else to build an abstraction library on top which only exposes the features that are available everywhere.

I might have a solution for that: dart-lang/language#416

Have we reached agreement here to move forward with the option described in #35969 (comment)?

I'm still worried that we'll go into all this trouble carefully refactoring the core libraries and telling our users to migrate, and in the end we'll end up with platforms that can only implement these libraries partially leaving us exactly where we started. We can only predict the future so far. Any rigid structure we come up with today is bound to get outdated.

I think there might be a way to have something more malleable that changes its shape as we onboard more platforms. However, it will require some design work, perhaps new language features. We might be better off doing something that feels hacky short-term and invest our energy into something that lasts.

@leafpetersen
Copy link
Member

I'm still worried that we'll go into all this trouble carefully refactoring the core libraries and telling our users to migrate, and in the end we'll end up with platforms that can only implement these libraries partially leaving us exactly where we started. We can only predict the future so far. Any rigid structure we come up with today is bound to get outdated.

The proposal in that comment isn't to refactor the core libraries, it's to expose the platform independent parts in a package. One of the key benefits of this is that packages are versioned, so there's a story for changing the functionality provided by the package if and when we add platforms.

@a-siva
Copy link
Contributor

a-siva commented Oct 3, 2019

Is this a D26 issue ?

@leafpetersen leafpetersen modified the milestones: D26 Release, Future Oct 3, 2019
@mit-mit
Copy link
Member

mit-mit commented Oct 7, 2019

Not D26, but I believe we need this no later than D27.

@vsmenon
Copy link
Member

vsmenon commented Oct 24, 2019

Pushing from D27 to next as we haven't nailed down what / requirements yet.

@ykmnkmi
Copy link
Contributor

ykmnkmi commented Feb 4, 2020

litle case:

import 'player_base.dart'
if (dart.platform.html) 'web_player.dart'
if (Platform.isWindows) 'winmm_player.dart'
if (Platform.isLinux) 'alsa_player.dart'
if (Platform.isMacOS) 'audiotoolbox_player.dart'
if (Platform.isAndroid) 'audiotrack_player.dart'
// ...
; ```

@lrhn
Copy link
Member

lrhn commented Feb 4, 2020

This case is interesting because it requires environment declarations in order to drive configurable imports.
That is an option too for the platform/OS value: Define a set of environment keys, like dart.platform.os which any implementation can set to match their run-time.

import 'player_base.dart'
   if (dart.library.html) 'web_player.dart'  // Already works!
   if (dart.platform.os == "windows") 'winmm_player.dart'
   if (dart.platform.os == "linux") 'also_player.dart'
   // ...
   ;

That comes with some complications too.

It means that platform agnostic compilation is impossible when you have to specify the target platform at compile-time, ... which you do because it affects which libraries are imported and compiled.

On the other hand, any new platform (like the sweet SugarOS platform) can introduce itself by simply setting the platform OS variable to, fx, "sugaros", and then code can slowly adapt to that also existing).

It's only an approach which works for fairly static things, and not something we can do for, say, supporting File operations.

@natebosch
Copy link
Member

OS specific imports are not something we want to support on the build system side. We will be doubling our "platforms" for compilation with NNBD, multiplying that again by the number of operating systems is not feasible.

cc @jakemac53

@MisterJimson
Copy link

Just want to chime in and mention that if this value was const like kIsWeb or kReleaseMode that would seem to mean that code could be optimized out when building for different platforms, reducing the amount of code included but not used in the final binary/build.

@jakemac53
Copy link
Contributor

Note that configurable imports also enable tree-shaking, but actually in a much more powerful sense. Only the matching import ever exists in the program at all. The other code never has to be tree shaken by the compiler because it is never imported. This includes all the transitive deps of each platform specific import.

@vsmenon
Copy link
Member

vsmenon commented Aug 7, 2020

@lrhn - should we close this in favor of a different issue re package:platform?

@mit-mit
Copy link
Member

mit-mit commented Aug 10, 2020

I believe we should, @lrhn ?

@lrhn lrhn added the closed-not-planned Closed as we don't intend to take action on the reported issue label Oct 2, 2020
@lrhn
Copy link
Member

lrhn commented Oct 2, 2020

The current plan is to provide a package which supports the behavior of Platform.operatingSystem, but which also works in a browser. See http://github.com/dart-lang/os_detect.

@lrhn lrhn closed this as completed Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue customer-flutter library-core
Projects
None yet
Development

No branches or pull requests