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

Support custom Resolvers in build.yaml #2345

Closed
shyndman opened this issue Jul 8, 2019 · 15 comments
Closed

Support custom Resolvers in build.yaml #2345

shyndman opened this issue Jul 8, 2019 · 15 comments

Comments

@shyndman
Copy link
Contributor

shyndman commented Jul 8, 2019

Dart: 2.4.0
build_config: 0.4.1
build_runner: 1.6.2

These packages are almost set up to allow custom Resolvers to be specified through build.yaml, but a couple more pieces are necessary. Would you consider adding them, or accepting a pull?

The reason I ask is because there's been a lot of discussion surrounding the analysis of dart:ui and Flutter (#733, #1436), and custom resolvers would be an excellent stopgap until you can rework build_resolvers to be more flexible.

Just a thought. Thanks!

@shyndman shyndman changed the title Support Support custom Resolvers in build.yaml Jul 8, 2019
@jakemac53
Copy link
Contributor

I think it might be enough to just provide a custom SDK summary file in your build.yaml? In general everything in a build.yaml should be portable across machines, so it would probably need to be a relative path under the SDK or something. I don't know if flutter ships with a custom summary file or not though today.

@shyndman
Copy link
Contributor Author

shyndman commented Jul 8, 2019

That would work too. How would you approach handling path differences across machines?

There are a couple summaries in flutter/bin/cache/dart-sdk/lib/_internal/, but they don't appear to contain dart:ui.

@jakemac53
Copy link
Contributor

How would you approach handling path differences across machines?

As far as path separators, we would always use / (the build system itself only deals in url style paths for consistency). We translate those to/from machine specific paths when necessary.

The paths would also be relative to the current SDK root to prevent machine specific absolute paths.

There are a couple summaries in flutter/bin/cache/dart-sdk/lib/_internal/, but they don't appear to contain dart:ui.

Anything under flutter/bin/cache/dart-sdk is just a copy of the actual Dart SDK at some version, so it won't contain dart:ui (which is why the default resolver doesn't work).

The summary for flutter would have to be shipped somewhere else under the sdk if it isn't already.

@shyndman
Copy link
Contributor Author

shyndman commented Jul 8, 2019

I see. Do you see including that summary as a possibility?

@jakemac53
Copy link
Contributor

jakemac53 commented Jul 8, 2019

I see. Do you see including that summary as a possibility?

My assumption is actually that they already do, I just haven't had a chance to check. The analyzer (in your IDE) must be able to somehow resolve dart:ui and that should be coming from a summary.

@shyndman
Copy link
Contributor Author

shyndman commented Jul 8, 2019

The analyzer resolves dart:ui through an _embedder.yaml in the Flutter package. You can find it here:

flutter/bin/cache/pkg/sky_engine/lib/_embedder.yaml

@shyndman
Copy link
Contributor Author

shyndman commented Jul 8, 2019

@jakemac53
Copy link
Contributor

I believe actually that this is handled in the analyzer by libraries.dart files (see https://github.com/dart-lang/sdk/blob/master/sdk/lib/_internal/js_dev_runtime/libraries.dart for instance).

It is possible though that one of those is generated off of the _embedder.yaml or something, or I am wrong about how that works.

Also afaik these files are essentially just used when generating a summary file.

cc @stereotype441 who would probably know more

@shyndman
Copy link
Contributor Author

shyndman commented Jul 8, 2019

_embedder.yaml is sort of an override for code you linked. If specified in a package's lib folder, you can use it to establish dart: URI mappings of your choosing.

You can even try it! Change that cached file to include a dart:bears mapping, restart your editor, and try importing it.

See, dart:ui isn't really a core Dart package. You won't find it in dart-lang/sdk. It's just made to look that way through this file.

That's my understanding anyway.

@shyndman
Copy link
Contributor Author

shyndman commented Jul 8, 2019

But I'm getting off-topic.

A summary would be great, but configurable resolvers would still be useful in any case, wouldn't they?

Edit: politeness 🙂

@jakemac53
Copy link
Contributor

Every feature incurs a maintenance/complexity cost so we want to make sure we are making the correct long term choices. Especially when it comes to the build.yaml format, it is very difficult to change it later on or remove old features. The intention is that people have a build_config dependency to handle this but not everybody does so in practice we can't change it without a lot of pain.

Configurable resolvers would still be useful in any case, wouldn't they?

Possibly, yes. That is a really large hammer though and also quite easy to get wrong in a way that would be bad for either build performance and/or build correctness. It isn't necessarily a door we want to open.

Generally we prefer to work from a problem statement rather than a solution. In this case it sounds like that would be something like "dart:ui isn't available with the default resolver". Another option for that specific problem would be a custom summary file.

@shyndman
Copy link
Contributor Author

shyndman commented Jul 8, 2019

Cool. Looking forward to seeing what you come up with.

@eernstg
Copy link
Member

eernstg commented Jul 30, 2019

@shyndman wrote

configurable resolvers would still be useful

Some kinds of code generation could use metadata to make decisions about whether to generate some code, or how to do it, and it could use other data like the value of constant expressions, including default values of parameters, from any reachable library. 'reflectable' does all of these, and it currently does not work with declarations in platform libraries in this respect, because there's no access to metadata, default values, and other AstNode related information.

So being able to set up a resolver that relies on libraries everywhere (and never uses a summary) might be a bit slower, but the trade-off is that it works. ;-)

So that would be very useful indeed!

@shyndman
Copy link
Contributor Author

#2501 fixes my original issue. Feel free to close this off unless you feel like the feature would be valuable regardless.

@jakemac53
Copy link
Contributor

jakemac53 commented Oct 15, 2019

Thanks! Ya I think that resolves the original need here.

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

No branches or pull requests

3 participants