Porting observe to Reflectable, fix #75

[dam0vm3nt]

A POC for fixing #75

[jmesserly]

The symbol->String change is great, BTW!

Main concern I have right now is around the Polymer dependency; we worked pretty hard to get this package to be platform-agnostic. Maybe @jakemac53 (or @sigmundch?) have thoughts on how to layer this. (I'm not familiar with jsProxyReflectable that comes from the Polymer package...)

[sigmundch]

I agree - it would be great to maintain that separation. One way to do so could be to introduce a second mirror system local to observe, different than the one from polymer, which only has one capability: to read and write properties marked with @observable.

The disadvantage of doing this is that there may be some duplication of data between observe's and polymer's mirror system. Can we work around it by adding a level of indirection and read the mirror system from a shared location, so we can exchange what mirror system we are using later on?

I'm still not sure what's the best way to do this, though. Ideally we should be able to do it in a way that lets dart2js get rid of the code of the observe mirror system. The idea would be that if you are using observe on its own, you end up using observe's mirror system, and if you use it with polymer, you replace the mirror system in that shared location and use the same mirror system everywhere (jsProxyReflectable).

@jakemac53 WDYT?

[dam0vm3nt]

I think that removing the dependency from polymer should not require user to mixin and annotate models twice (once for polymer with jsproxy and reflectable and once for observe). That's why I choose to reuse polymer mixin and annotation.

To me the best choice would be to refactor the mixin and the annotation to observe and make polymer depend on it.

[dam0vm3nt]

... Or create a general reflectable use cases package that both depends on ...

[jakemac53]

I think its probably best to keep a custom reflectable annotation for Observe, and have everything go through that. This does mean you have to mark things as both @observable and @reflectable, but we might be able to remove that limitation in the future.

Overall the size impact should be pretty minimal, its just the size of the reflection code itself which would be extra. It shouldn't cause any additional issues with tree shaking because all the same things are already included because of @reflectable.

[dam0vm3nt]

What about using the transformer to choose with mirror system to use ? We can add a parameter in pubspec to instruct the transformer to replace Observable and @observable with JsProxy and @reflectable only when you choose to do that. The transformer would also implement a simple reflector method to be use to obtain the InstanceMirror. Otherwise it will use its default annotation and mixin. This way we have only one mirrorsystem and even if metadata is small (it is?) it is not a good idea to double it.

[dam0vm3nt]

@jakemac53 would you agree to create a tiny meta package on which polymer should depend that exposes a meta annotation to be used by observe to discover the mirror system to use at runtime? Something like this :

class MetaReflectorScope{
  List<String> scopes;
  const MetaReflectorScope(this.scopes);
}

class MetaReflector extends Reflector {
  const MetaReflector() : super.fromList(const TopLevelInvokeMetaCapability(MetaReflectorScope),const InstanceInvokeCapability("reflect"));
}

const metaReflector = const MetaReflector();

On polymer :

@metaReflector
class JsProxyReflectable extends Reflectable {
...

@MetaReflectorScope(["observe","polymer"])
const jsProxyReflectable = const JsProxyReflectable();
...

At this point on observe MetaReflector can be levereged to find the reflector it needs while on polymer nothing else changes.

I think this way we can win two prizes and it is an interesting application of reflectable itself.

WDYT ?

[dam0vm3nt]

Thinking of it ... the meta structure I depicted in the comment before could actually be incorporated in reflectable package itself... along with a method like : findReflectorWithScopes(List<String> scopes)

[jakemac53]

I definitely agree that some sort of meta-reflection should exist on the reflectable side of things, although I don't fully understand how the one you describe works (how does observe discover jsProxyReflectable?).

I think that in the shorter term observe could also just expose an imperative api for registering reflectors for different types globally. Something like registerReflector(Type type, Reflectable reflectable). That would allow polymer_autonotify to register both JsProxy and PolymerMixin with jsProxyReflectable, and there are no direct dependencies between any of the packages.

[dam0vm3nt]

OK great I'll work on a proof of concept. The idea behind it is to use reflection itself to discover reflector but I'm not sure it is feasible.

[eernstg]

It is no problem to support a Reflectable.reflectors static method which returns the set of all reflectors in the system, that is, the set of instances of a subclass of Reflectable which is actually used as a reflector by being attached to a target class as metadata or by being used in a global quantifier. This is also the set of reflectors for which data has been generated. I think that a reflector with the empty set of target classes will still work even though we probably haven't tested it, you will just discover that it has an empty set of supportedClasses; anyway, if you hit a bug here we'll check it out. ;)

With that set, it is easy to go through the reflectors and check which capabilities they have (myReflector.capabilities), and that enables you to select a reflector with suitable capabilities. However, you also need to consider which classes are covered by each reflector such that it will be able to work on the classes you care about, but you can check that with myReflector.annotatedClasses (well, that method ought to be renamed to supportedClasses because they aren't annotated if they were included by any kind of quantification).

Wouldn't that fit into your scenario?

There is no need for your usage of a given reflector to use const, you could very well have final Reflectable myReflector = selectASuitableReflector(Reflector.reflectors); and then use that. You may drop final if you wish, but it might be nice to ensure that you are not switching from one mirror system to another one by accident.

I've created a CL where such a Reflectable.reflectors method is available: https://codereview.chromium.org/1391543003/. Please create an issue on reflectable telling us that you want that feature, if it is indeed what you need.

[eernstg]

It just came to mind that the dependency you describe ("observe wants a mirror system, but it will get it implicitly via Reflectable.reflectors from some other library L where the chosen reflector comes from") allows observe to use the set of classes selected by that other library L. This means that it is a feature rather than a problem that the selection is made by L and used by observe. Do you agree?

[dam0vm3nt]

Here is what I had in mind:

• reflectable : add a meta reflector like this
• polymer-dart : publish a mirror system to be used for certain scopes : see here

On 'observe` then just resolve the reflector in this way :

resolveReflector() {
 var r = findReflectorsForScope("observe");
 if (r.isNotEmpty) {
  return r[0];
 }
 return defaultObserveReflectable;
}

A "scope" here should define a contract between the reflector user and the reflector provider. This mean that it is the provider responsability to provide a reflector with the right capabilities, suitable to be used in the "scope" it is required.

@eernstg : using this approach there is no need to implement any static method. Note that even the meta reflector can be exported in the same way (I've used the scope "meta" for this). I had to use a static method on a class to export the reflector because top level variable (and const) are not yet supported... am I right ?

@jakemac53 : this is equivalent to implementing a static registry but it levereges the reflectable package to create the registry automagically. Just annotate a class and estabilish a "scope" convention.

If this approach is worth of been discussed anymore I will send you PR, create issues to packages and go on that way.

And yes: this is definitively a feature request for both reflectable and polymer-dart ... and an issue for observe porting to reflectable.

FWIW : I've tested those changes on a little demo and it works : reflectors are found and can be used.

[eernstg]

We just published a new version 0.3.2 of reflectable where you have support for this kind of "meta-level reflection".

The only required extra feature is a new Reflectable.getInstance static method which will return the canonical instance of a given reflector type. So you have a type like MyReflectable (or myReflector.runtimeType), and you want the corresponding const object const MyReflectable() (respectively const SomeOtherClassThatWeDontKnowStaticallyReflectable(), which is the case that you can't express without the new static method).

You can take a look at the test case 'meta_reflectors_test.dart' (and a handful of other libraries in the same directory which are imported from there) in order to see how you can implement a reflector (AllReflectorsMetaReflector) which is able to give you access to all the "mirror systems" available in your program, and another one (ScopeMetaReflector) which shows how you can "register" functions anywhere in your program (by adding @ScopeMetaReflector() to them) as mappers from String to Set<Reflectable>, such that you can get the set of reflectors in a given scope, in a similar sense as in your examples.

The 'meta_reflectors_test.dart' program has been modularized extensively in order to show that various things do not need to depend on each other (concretely, various libraries do not import specific other libraries). E.g., Reflector, Reflector2, ReflectorUpwardsClosed need not have any metadata and do not need an import of anything locally, and the library where reflection is used does not have to import the reflector defining libraries, etc. Basically, all the pieces can be brought together in the main file 'meta_reflectors_test.dart', and no other library needs to import that.

I think this will suffice for a broad range of approaches including the one that you described.

[eernstg]

About reflectable and support for top-level variables: I've implemented a bunch of stuff in relation to libraries (they used to be covered in a quite incomplete manner) and this should cover top-level variables as well. I just made a note to self that this hasn't been tested, though. ;-)

[dam0vm3nt]

@eernstg wow! that's great. I'll post a feature request on polymer-dart to add registering the polymer reflector for a suitable scope to be used by observe.

[eernstg]

Thanks! Note that reflectable as such only adds the seemingly benign method getInstance, but the example that I mentioned shows how scopes and stuff can be implemented outside of reflectable based on that. The ScopeMetaReflector may or may not be what is needed here, but it should be a usable starting point. I don't know if there is a need for a new package, but if polymer imports observe then observe could contain it and polymer could use it.

[dam0vm3nt]

I've removed polymer dependency. For now I'm giving up trying to reuse its mirror system.

[eernstg]

Removing the polymer dependency from observe sounds fine, but you shouldn't have to give up on reusing its mirror system (assuming that it will do what you need). I've added a comment on https://github.com/dart-lang/polymer-dart/issues/620 which illustrates how we envisioned that the new getInstance method could be used to enable just that.

[dam0vm3nt]

ì@eernstg I've read your comments but if I have understood it right then anyone who wants to reuse polymer mirror system should add something like this :

 Map<String, Iterable<Reflectable>> scopeMap = <String, Iterable<Reflectable>>{
    "observe": <Reflectable>[jsProxyReflectable]
  };

  @ScopeMetaReflector()
  Iterable<Reflectable> reflectablesOfScope(String scope) => scopeMap[scope];

to their code. Or at least (more preferably) we should create another package, something like polymer_observe_mirror_binding that will depend on polymer (for jsProxyReflectable) and observable (for @ScopeMetaReflector thing) that will contain that binding.

Initially my idea was that this kind of binding could be exported by polymer itself. But I imagine that just make sense to have it on a separate package.

[dam0vm3nt]

Obviously for polymer to export such a binding and not depend on observe or another package @ScopeMetaReflector had to be incorporated in reflectable API.

[eernstg]

About reusing the polymer mirror system: For any piece of code where an import of 'package:polymer/src/common/js_proxy.dart' is acceptable the use of jsProxyReflectable is up for grabs. ;)

The extra difficulty in relation to observe is that you don't want to depend on polymer in general (only in the bridge library), so the connection needs to be established dynamically, and that amounts to a model for using various different "foreign" mirror systems, as long as they will deliver the required services.

For users of the observe package who want to use it with polymer, there should not be a need for any extra incantations, because the definition of reflectablesOfScope is provided by 'polymer_autonotify'.

That particular binding could also be made by a separate library (say, 'polymer_observe_mirror_binding'), if it is sufficiently reusable for others than 'polymer_autonotify' (and its users).

[dam0vm3nt]

Now it observe will be able to reuse other mirror system. I've updated the readme too. Again tested only on basic test.

Default mirror system is bundled with observe, just need only to import observe/standalone.dart in place of observe/observe.dart.

Updated polymer_autonotify to reuse polymer-dart mirror system when using observe.

[eernstg]

Cool!

[dam0vm3nt]

Yes, Thank you for your help. Do you think there is something else to be done to have this pull request merged?

[eernstg]

It might be nice for users of stand-alone observe to keep importing 'observe/observe.dart', and users of observe-with-something would import something else (a bridge library like 'polymer_autonotify.dart') anyway, so they won't notice if the old 'observe.dart' is renamed. So how do you think the numbers are, would stand-alone users of observe plus users of bridges outnumber the providers of bridge libraries so massively that 'observe/standalone.dart' should be renamed to 'observe/observe.dart' and the old 'observe/observe.dart' should be renamed to something else? (maybe 'observe/mirror_system_configurable_observe' if it hadn't been so darned long ;-)

One thing that I noticed was that the code uses fromList in the super-initializer in reflector classes; there is no need to do that unless you have "many" elements in the list, because Reflectable has been equipped with a constructor that takes up to 10 arguments directly and makes a list of them.

[dam0vm3nt]

That was something I was thinking about.

In my opinion - but probabily others knows better than me - the vast majorty of users of observe are using it together with polymer. So I expect they prefer to have the mirror less one (observe_bare.dart ?) be left as observe.dart so they don't have to change their import in all their model classes.

[eernstg]

OK, I didn't realize that they would import 'observe.dart', I thought that 'polymer_autonotify.dart' would take its place when observe is used with polymer (but I can see that it doesn't export 'package:observe/observe.dart'). In that case there are lots of people depending on the name 'observe.dart' so renaming is probably not a good idea.

[dam0vm3nt]

I agree. I think there is an high confidence that a lot of people created libraries with only model objects thus importing only "package:observe/observe.dart" even though they will use those objects with polymer. This is a very common pattern to be found.