dart-archive / polymer-dart

Polymer support for Dart
https://pub.dartlang.org/packages/polymer
BSD 3-Clause "New" or "Revised" License
180 stars 33 forks source link

Support reflectable 0.3.2 and register polymer mirror system for `observe` #620

Closed dam0vm3nt closed 9 years ago

dam0vm3nt commented 9 years ago

Following discussion on https://github.com/dart-lang/observe/pull/76 it would be great if polymer-dart could register its mirror system using a scope suitable for future observe package integration.

jakemac53 commented 9 years ago

Ideally nothing would need to be done by Polymer here since it does not actually itself depend on observable. The ideal solution would be if this could live purely in the polymer_autonotify package, since that is what is actually introducing the dependency on observe. That being said its not out of the question if its not possible to do it that way, but I would like to explore some option which does make that clean separation possible.

eernstg commented 9 years ago

Given that 'meta_reflectors_test.dart' builds the mapping that lets 'meta_reflectors_user.dart' use the reflectors defined in 'meta_reflectors_definer.dart' and 'meta_reflectors_domain_definer.dart', and none of them depend on 'meta_reflectors_test.dart', directly or indirectly, it should definitely be possible to add a suitable reflector scoping mechanism to 'polymer_autonotify'.

dam0vm3nt commented 9 years ago

If ScopeMetaReflector could be incorporated in the main reflectable package then polymer-dart could use it to declare its "willingless" to provide a mirror system suitable of been used as an observe mirror system, i.e. provide @jsProxyReflectable in scope observe. It will not depend on the observe package or on poylmer_autonotify, but only on reflectable.

observe can then be modified in a way that it will take advantage of existing mirror system if there is one by using ScopeMetaReflector to find a reflector for its scope. Otherwise it will use one of its own.

This way when observe is used together with polymer-dart there will be only one mirror system and metadata will not replicated. Also there would be no reason to reannotate each method / property.

eernstg commented 9 years ago

Here's what you could do in order to make it possible for 'observe' to dynamically discover and use a reflectable mirror system without depending on polymer: Add the following to 'observe/src/metadata.dart' (that's a verbatim copy from the 'meta_resolvers_meta.dart' I mentioned earlier):

  class ScopeMetaReflector extends Reflectable {
    const ScopeMetaReflector()
        : super(const TopLevelInvokeMetaCapability(ScopeMetaReflector),
              declarationsCapability, libraryCapability);
    Set<Reflectable> reflectablesOfScope(String scope) {
      Set<Reflectable> result = new Set<Reflectable>();
      for (LibraryMirror library in libraries.values) {
        for (DeclarationMirror declaration in library.declarations.values) {
          result.addAll(library.invoke(declaration.simpleName, [scope]));
        }
      }
      return result;
    }
  }

This uses a string-to-set based policy, as in your description of the idea, but you are of course free to use a different policy if that turns out to fit better (you could insist on delivering a single reflector rather than a set and throw an error if you have more than one available, or you could map from some notion of a query rather than from a string, etc).

In any case, you may now get hold of your reflector by querying ScopeMetaReflector rather than by creating a const ObservableObject():

  final Reflectable observableObject = 
      const ScopeMetaReflector().reflectablesOfScope("observe").first;

Now, observableObject is an alias for whatever reflector we choose to make available for the ScopeMetaReflector.

Note that we do not depend on (that is, import) anything here that reveals where that reflector might come from. It could be from polymer or it could be from anywhere else. In a context where 'polymer_autonotify.dart' is used and we want to connect observe and polymer, we would add the following to 'polymer_autonotify.dart':

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

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

With this, 'polymer_autonotify.dart' connects polymer to observe because it allows observe to discover and use jsProxyReflectable, and it does not require any dependencies between polymer and observe.

This easily generalizes to the relationship between observe and X for other X'es: If you wish to use, say, a reflector associated with a serialization library in observe then you can just create a similar "bridge" library (say, 'serialization_autonotify.dart') which offers a reflector from there for observe to use.

If you wish to use your own reflector (something like the current ObservableObject then you could simply introduce a "null bridge", maybe called 'standalone_autonotify.dart' which contains

  class ObservableObject .. // same as your current class of that name
  const observableObject = const ObservableObject();

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

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

This requires that users of observe who are using it in a stand-alone manner (i.e., not with anything else that already provides sufficient reflection support) must now import the "null bridge" library rather than 'observe.dart', but you could rename the libraries to get the same structure and keep 'observe.dart' as the name to import for stand-alone usage (other users would keep importing 'polymer_autonotify.dart' so they don't care that it's renamed).

eernstg commented 9 years ago

Ah, just noticed one more thing: You said 'observe can then be modified in a way that it will take advantage of existing mirror system if there is one by using ScopeMetaReflector to find a reflector for its scope. Otherwise it will use one of its own.'

There is no way you could save space if you want to make a choice at runtime about whether or not you'd use your own reflector: If it is present in your program then you will pay for it in terms of space, and there is no way a successful invocation of reflectablesOfScope or anything else at runtime could enable you to save that space (the data cannot be deleted because you might start using "your own" reflector at any point).

So if you want to save the space you will have to make the decision ("use jsProxyReflectable rather than my own ObservableObject?") in terms of your import graph, not based on of any runtime decision. That's the reason why you need the bridge libraries as I described them (but since 'polymer_autonotify.dart' is already such a bridge library, and there might be others, it won't make any difference for the users of those libraries).

dam0vm3nt commented 9 years ago

@eernstg regarding this last point, I'll make you one question: what will happen if observable declares a Reflectable and the user do not use it in favour of polymer one ? Will it take space? I thought not but I may be wrong.

dam0vm3nt commented 9 years ago

obviously the answer to my previous question depends on the capabilities of the reflectable declared byobserve. I have to specify that it will contain only InstanceInvokeMetaCapability and little more.

eernstg commented 9 years ago

About the space: If package observe declares a reflector class (a subclass of Reflectable whose canonical instance is intended to be used as a mirror system) and uses annotations to associate that reflector with some classes/members, then the transformer will have to generate the corresponding mirrors and you cannot get rid of them, so you will have to pay for them in terms of memory usage. It doesn't help if you never use it, because it is reachable via a top-level variable (and it would be a terrible mess if anyone outside reflectable started assign [null] to that variable, just because they think its value won't be needed ;-). Besides, program size may actually be more important than runtime space consumption, and the program certainly will have to contain the generated mirror code.

But if package observe declares a reflector class and nobody associates that reflector (say, observeReflector) with anything (by not annotating any classes with it directly as metadata, and not using it in global quantifiers) then it will be a very cheap mirror system, it will just get a few bytes of generated code with some empty lists and maps.

So it depends on who will make that choice: Do you expect that you'll be able to avoid associating anything with that reflector, or do you quantify over all subtypes of something that users cannot avoid using? (Seems like ChangeNotifier and all its subtypes are included no matter what, so it depends on how many subtypes of ChangeNotifier you will have to have).

dam0vm3nt commented 9 years ago

The idea was to remove the quantification. But this also means itroducing a breaking change because observe users will have to change their code if they use observe standalone.

eernstg commented 9 years ago

By the way, about the use of both @observable (from package observe) and @reflectable (from package polymer): I can't believe that it is necessary to have both on the same members, but I don't know enough about the situation as a whole be able to see where the difficulties arise. But I suppose it is not something that package observe needs anyway, as soon as the set of members "to be aware of" has been settled, package observe doesn't care whether it was done using one or the other mechanism.

eernstg commented 9 years ago

About the breaking change: Would it be inconvenient if stand-alone users of observable were required to import a "standalone wrapper" library, similar to 'polymer_autonotify.dart' .. and possibly named 'observe.dart'? (and then the non-stand-alone users would be so few and so well within reach that they could be persuaded to use a new name..)

dam0vm3nt commented 9 years ago

regarding the double annotation : At the moment observe and polymeruse each their own mirror system this is the reason we have to double annotate each member. (I am assuming here that two mirror system do not share data).

if we had only one mirror system then one could use only the polymer one.

dam0vm3nt commented 9 years ago

about breaking change : apart from having to import a "bridge" package the problem is that at the moment an Observable object just have to extend or mixin the Observable mixin. The transformer will replace it with a ChangeNotifier that is quantified and make the objects metadata available.

If we remove quantization because we do not know the mirror system in advance then the user of observe standalone should also annotate each of their object with something suitable for the standalone mirror system, i.e.: they will have to add @observableObject to their objects.

eernstg commented 9 years ago

I don't see why using two different mirror systems causes a need for member selection metadata to be of different types. You can use any type (that is capable of const construction) in InstanceInvokeMetaCapability and its ilk.

eernstg commented 9 years ago

You can put a global quantifier into the "bridge" layer and use that to annotate ChangeNotifier and all its subtypes with whatever reflector you want. That would enable you to annotate it with jsProxyReflectable when used with polymer, and with something else in other cases.

You would just need to standardize on what kind of metadata you want to use for selecting members.

dam0vm3nt commented 9 years ago

mmh. You mean avoid using subtypeQuantification at all? Sounds interesting.

Then we will have:

Sounds right ?

dam0vm3nt commented 9 years ago

before I meant avoid using "metadata quantification"...

eernstg commented 9 years ago

You can use global quantification and subtype quantification together. Check out the example 'meta_reflector_test.dart' in reflectable. It uses a global quantification:

@GlobalQuantifyCapability(
    r"^reflectable.reflectable.Reflectable$", const MetaReflector())

to request that class Reflectable be annotated with const MetaReflector(), and that reflector itself uses subtype quantification to cover all reflectors.

eernstg commented 9 years ago

Sounds right: objects just have to have annotations suitable for the polymer context in order to be usable with observe-via-polymer_autonotify.

The stand-alone scenario also sounds right.

eernstg commented 9 years ago

About the metadata quantification: I would still assume that selection of members would require some annotations, unless it's ok to make all members available for observation. I guess that would be too expensive?

dam0vm3nt commented 9 years ago

Yes indeed. Polymer @reflectable for the polymer use case and @observable for the standalone case.

But I forgot the observe transformer. It needs the "@observable" annotation to distinguish which fields have to be replaced by getters and setters calling notifyPropertyChange.

This means that polymer users should still have to annotate their classes twice, In this case the "@observable" annotation won't be used by the mirror system and only used as a marked for the transformer. But this can be simplified by a trasformer in autonotify (there is already one that does this BTW).

eernstg commented 9 years ago

Couldn't the observe transformer receive information about which type of metadata to look for, somehow? Otherwise, there is an even greater need to standardize on the member selection metadata type with polymer, such that both can use the same type, and none of them need to depend on the other for that.

eernstg commented 9 years ago

(By 'somehow', I was thinking about something like ..MetaCapability, for instance, adding a piece of metadata to the import of observe.dart or something similar where the argument is the desired type.)

dam0vm3nt commented 9 years ago

it could also be just a parameter for the transformer configuration.

Having the transformer check for an annotation on observe import sounds to me to cumbersome. But I'm not very expert in writing transformers.

eernstg commented 9 years ago

If it could be just an extra couple of words in pubspec.yaml then it might not be a problem.

dam0vm3nt commented 9 years ago

something like:

transfomers:
- observe:
    with_annotation: "@reflectable"
...
dam0vm3nt commented 9 years ago

thanks to @eernstg I succeeded in making observe reuse polymer-dart mirror system even without any changes to polymer-dart. So I'm closing this issue.

jakemac53 commented 9 years ago

Great!

dam0vm3nt commented 9 years ago

tnx!