Skip to content
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

Drop support for Isolate.resolvePackageUri from dart2js and DDC #37153

Closed
natebosch opened this issue Jun 4, 2019 · 3 comments
Closed

Drop support for Isolate.resolvePackageUri from dart2js and DDC #37153

natebosch opened this issue Jun 4, 2019 · 3 comments

Comments

@natebosch
Copy link
Member

The implementation in dart2js is broken by default already. It was broken in 7911fb2#diff-894e25213751e7522ad6394c40e9378e

Unless the user explicitly assigns to defaultPackagesBase the call will fail. We do have some tests against this behavior:

context['defaultPackagesBase'] = 'path1/';

There is at least one known use case which attempts to be cross platform which depends on this. It's currently broken by default with dart2js but works with DDC: https://github.com/dart-lang/resource/blob/f8e37558a1c4f54550aa463b88a6a831e3e33cd6/lib/src/resolve.dart#L11

We need to decide if it is worth supporting this use case and either break support, or migrate the implementation to some other place - perhaps as a utility in dart:html. My preference would be to remove support.

cc @vsmenon @sigmundch

@sigmundch
Copy link
Member

Interesting, while we can't rule out that people are using it via defaultPackagesBase, it does feel less likely.

My inclination is to go ahead and remove the support as well.

We can do a quick search for packages that use either the isolate API or the resolve package to assess potential impact to make sure. Then create a breaking change proposal to remove the support. Especially since we say that dart:isolate itself is not supported on Dart for the web, we can clarify that this is extra clean up.

Another option to consider here is that instead of removing just this API, to go to the full leap and make it invalid to import these libraries entirely. That has the potential of being more breaking (since the presence of an import will break people's builds), so I'd consider that more cautiously.

@lrhn
Copy link
Member

lrhn commented Jun 4, 2019

If dart:isolate is not supported, we should not allow importing it. Then noone can access Isolate.resolvePackageUri anyway.

We have made yet another "kind-of supported" library, and we shouldn't be doing that.

What would break if we simply disallowed importing dart:isolate in web compilers? (Along with dart:io)

@natebosch
Copy link
Member Author

we should not allow importing it

The golden path for external users is to compile through build_runner with build_web_compilers which does not allow importing dart:isolate already - although there is a bug if the import is in the same file as main, we should have that fixed soon.

We are working hard on restricting imports internally as well. dart:mirrors is currently on a whitelist, dart:isolate should also be on a whitelist soon, dart:io is disallowed.

Users who run dart2js directly do not have this restriction. I'd be in favor of adding it, but we'd need to figure out if any workflows will be broken and we'd still need an escape hatch internally for some legacy code which can't easily be refactored to avoid the imports.

We have made yet another "kind-of supported" library, and we shouldn't be doing that.

We did that for dart:mirrors, dart:io, and dart:isolate already. We've been working on slowly closing those gaps ever since.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants