source_gen icon indicating copy to clipboard operation
source_gen copied to clipboard

Add a note about not importable annotations

Open natebosch opened this issue 3 years ago • 12 comments

The build systems run on the Dart VM only, so there are limitations on what imports can be used by builders. Add documentation describing how to work around this limitation by overriding the TypeChecker with something other than fromRuntime.

natebosch avatar Dec 03 '21 21:12 natebosch

Should we just provide a GeneratorForMatchingAnnotation which takes a TypeChecker?

Just as a factory even, would be great. A follow-up is good here.

kevmoo avatar Dec 03 '21 21:12 kevmoo

Should we just provide a GeneratorForMatchingAnnotation which takes a TypeChecker?

Just as a factory even, would be great. A follow-up is good here.

The existing design imposes an extends already - you are intended to override generateForAnnotatedElement.

It would be nice to change that and allow composition but 🤷‍♂️

Given the current design, I think @override fits. WDYT?

natebosch avatar Dec 03 '21 22:12 natebosch

Given the current design, I think @override fits. WDYT?

Ya the design I would envision is making a base class that just takes a type checker, and then the existing class would just extend that to make the type checker for you.

I wouldn't change anything about how it works generally, just which class you extend would change (and what you pass to the super constructor)

jakemac53 avatar Dec 06 '21 15:12 jakemac53

which class you extend would change (and what you pass to the super constructor)

Adding another class to the public API would increase the surface area of the package and IMO make things harder to find.

natebosch avatar Dec 06 '21 16:12 natebosch

Adding another class to the public API would increase the surface area of the package and IMO make things harder to find.

🤷‍♂️ I don't really agree, I think its significantly better than doing the whole GeneratorForAnnotation<void> thing and then overriding the type checker. Overriding the type checker in that way is definitely a bit funky, and you are relying essentially on how that thing is being used internally in the super class...

jakemac53 avatar Dec 06 '21 17:12 jakemac53

I'm not able to find a design I like here with another class (especially because I can't think of a nice name).

With a breaking change I could imagine something different, maybe where we drop the generic and you need a constructor when forwards to super.forType(someType) or super.forTypeChecker(someTypeChecker). If we're going with a breaking change we may as well drop the need for extends entirely though...

natebosch avatar Dec 14 '21 01:12 natebosch

@natebosch – thoughts on this?

kevmoo avatar Sep 13 '22 17:09 kevmoo

Uh...this is old @natebosch

kevmoo avatar Apr 05 '23 17:04 kevmoo

@jakemac53 - do you think this is worth landing as is?

natebosch avatar Apr 05 '23 17:04 natebosch

I still really don't like the GeneratorForAnnotation<void> suggestion personally. Should we instead just tell people to split up their annotation libraries so they don't have those imports?

jakemac53 avatar Apr 05 '23 18:04 jakemac53

Should we instead just tell people to split up their annotation libraries so they don't have those imports?

I don't know if that is always feasible - some annotations could use dart:ui classes as arguments.

natebosch avatar Apr 05 '23 20:04 natebosch

@jakemac53 - is this implementation of GeneratorForMatchingAnnotation match what you were thinking?

natebosch avatar May 09 '23 22:05 natebosch