package:os_detect doesn't work for tree-shaking

[jonasfj]

When using if (isWindows) { ... } from package:os_detect I would expect { ... } to be tree-shaking when I'm not compiling for Windows.

I think this is relevant for Flutter applications.

This works as expected when using Platform.isWindows from dart:io, however, importing dart:io is ill advised if developing packages that should be compiled for web.

If possible, maybe we could add a crude test case for this.

Example where tree-shaking is working

import 'dart:io' show Platform;
//import 'package:os_detect/os_detect.dart' as Platform;

void main() {
  if (Platform.isLinux) {
    print('LINUX ONLY MESSAGE');
  } else if (Platform.isWindows) {
    print('WINDOWS ONLY MESSAGE');
  } else {
    print('OTHER PLATFORMS MESSAGE');
  }
}

If I do:

$ dart compile exe --target-os linux bin/sayhello.dart 
Info: Compiling with sound null safety.
Generated: /usr/local/google/home/jonasfj/hacks/tree_shaking_playground/bin/sayhello.exe
$ cat ./bin/sayhello.exe | grep 'WINDOWS ONLY' > /dev/null && echo 'has windows messages'
$ cat ./bin/sayhello.exe | grep 'LINUX ONLY' > /dev/null && echo 'has linux messages'
has linux messages

It's a bit crude, but be binary doesn't contain the string WINDOWS ONLY MESSAGE, that indicates that tree-shaking works.

---

Example where tree-shaking is NOT working

//import 'dart:io' show Platform;
import 'package:os_detect/os_detect.dart' as Platform;

void main() {
  if (Platform.isLinux) {
    print('LINUX ONLY MESSAGE');
  } else if (Platform.isWindows) {
    print('WINDOWS ONLY MESSAGE');
  } else {
    print('OTHER PLATFORMS MESSAGE');
  }
}

If I do:

$ dart compile exe --target-os linux bin/sayhello.dart 
Info: Compiling with sound null safety.
Generated: /usr/local/google/home/jonasfj/hacks/tree_shaking_playground/bin/sayhello.exe
$ cat ./bin/sayhello.exe | grep 'WINDOWS ONLY' > /dev/null && echo 'has windows messages'
has windows messages
$ cat ./bin/sayhello.exe | grep 'LINUX ONLY' > /dev/null && echo 'has linux messages'
has linux messages

We see that the string WINDOWS ONLY MESSAGE is in the binary, indicating that it wasn't tree-shaken.

---

cc @sstrickl

[lrhn]

The os_detect package is built to allow mocking/faking/overriding the current data. That means that all reads go through an indirection, and you cannot possibly tree-shake anything.

You can get tree-shaking or dynamic overriding, but never both. (Unless the only way to override is command line flags like -Ddart.library.io.override.platform.operatingsystem=linux, then we can possibly make it compile-time constant.)

Or unless we can switch imports based on whether it's a debug or production build,

export "direct_os_detect.dart"
  if (dart.compilation.is_debug_mode) "mockable_os_detect.dart";

so overrides only work when compiling in debug mode.

But that makes production run different code than debug builds, which means we now need production-mode tests too. Which cannot use the mocks.

[mraleph]

The os_detect package is built to allow mocking/faking/overriding the current data. That means that all reads go through an indirection, and you cannot possibly tree-shake anything.

I think users don't really need mocking in production builds - they need most compact production builds possible. So we should operated based on that assumption.

I have vague recollection that we have already discussed this with @mit-mit and @mkustermann and we arrived to the conclusion that os_detect should be designed in a way that makes tree-shaking of production builds possible.

[jonasfj]

I think @lrhn and I came up with a design that might allow for tree-shaking while maintaining the ability to mock (provided that you don't use the mocking APIs as that would break tree-shaking).

[lrhn]

Yep, I was wrong, it is possible. Jonas had the inspired idea of using instantiation of classes to "remember" constant-ish knowledge in the tree-shaker.

I've uploaded a new patch to os_detect which seems to successfully tree-shake based on both isLinux and operatingSystem == "linux" guards, and still allows you to introduce a custom OS id string and version for testing, at which point tree-shaking will likely be less succeessful.

https://github.com/dart-lang/os_detect/pull/31

As long as you only use the default, using VM platform-constant values for guards allows us to ensure that exacly one of a number of classes is instantiated, so the rest can be tree-shaken, and then we can later rely on knowning the exact class of an object that is used in other tests. (I might have overdone the @pragma("vm:prefer-inline") annotations.)

I don't know where the limts of this approach is, but for small examples it works.

[rakudrama]

If using package:os_detect is made to work for tree-shaking for Flutter on VM, it should be made to work for Flutter Web too.

[lrhn]

It just might already work.

On the web, the only subtype instantiated is the BrowserOS type, so if web tree-shaking and inlining is good enough, it should recognize that an ... is LinuxOs will always be false, and the branch can be tree-shaken. Similarly doing .id on that type will always resolve to a final String id = "browser";, so comparing it to "browser" should again be detectable as true in the compiler.

Maybe we need to add some "inline" annotations for JS too.

[rakudrama]

It just might already work.

On the web, the only subtype instantiated is the BrowserOS type,

I see, the new 'magic' code is inside a conditional import. That is promising.

so if web tree-shaking and inlining is good enough,

🤣

it should recognize that an ... is LinuxOs will always be false, and the branch can be tree-shaken. Similarly doing .id on that type will always resolve to a final String id = "browser";, so comparing it to "browser" should again be detectable as true in the compiler.

Maybe we need to add some "inline" annotations for JS too.