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

Dart & Henson 3 Specs draft. RFC #164

Open
stephanenicolas opened this issue Oct 10, 2017 · 9 comments
Open

Dart & Henson 3 Specs draft. RFC #164

stephanenicolas opened this issue Oct 10, 2017 · 9 comments
Assignees
Milestone

Comments

@stephanenicolas
Copy link
Collaborator

stephanenicolas commented Oct 10, 2017

Hi @f2prateek , we wanna change Henson and release a new version. Dart would be changed optionally as well, see below.

The problem we are facing is that Henson doesn't, and cannot in its current form, work well with modules:

  • if 2 activities in 2 different modules want to navigate to each other, then Henson cannot handle that because it would create a compilation cycle between the modules to reflect the navigation cycle, which is legitimate.

We have found a work around, and we ask for your comments on this. We want to move fast.

Proposed Henson API changes

We are gonna change Henson to work completely the opposite of its actual state:

  • instead of generating intent builders from annotated activities that can refer to a navigation model, we want to generate the same intent builders from an annotated navigation model that will refer to the activity.
  • it will mean people who use multiple modules will have to create a separate module, or more, to contain the navigation models and use henson on this module. This navigation module will have the following properties:
    • it will not have any dependency on any other module
    • it will be the first to be compiled, other modules will depend on it
    • models will use an annotation to be detected/processed by henson and the annotation will refer to the name of the target activity, using its FQN, not the class as it would introduce a cyclic dependency.
    • the Henson class will be generated in the same way as before and will contain methods to obtain all intent builders
    • variant and flavors can differ and generate different Henson classes
    • we didn't start working on it yet, but we think this work will put us in a much better position to tackle instant apps navigation as well as modularization is an essential part of instant apps.
    • $$IntentBuilders will be renamed to __IntentBuilders to follow ButterKnife conventions and remove the ambiguity with inner classes
    • we are gonna refactor the code to make it cleaner at the annotation processor level
    • we are considering using Auto services in Dart, also as standard

Proposed Dart API changes

  • We are also thinking of renaming the @InjectExta annotation to @BindExtra in the same release. It will follow more closely the evolution of Butterknife and will disambiguate with DI annotations which are confusing for junior devs.
  • we will also change the name of the dart binding method
  • the $$ExtraInjectors will be renamed to __ExtraBinders and remove the ambiguity with inner classes

Please note that all this work will also make it easier to work on an incremental version of Dart & Henson.

@stephanenicolas stephanenicolas added this to the 3.0.0 milestone Oct 10, 2017
@stephanenicolas stephanenicolas changed the title Dart & Henson 3 Specs. RFC Dart & Henson 3 Specs draft. RFC Oct 10, 2017
@f2prateek
Copy link
Owner

Thanks for writing this up, seems like you have a good plan here, so go ahead! The Dart API changes look great to me :)

@fansmc
Copy link

fansmc commented Oct 31, 2017

if dart3.0 support custom annotation?
like this:

  • in annotation
    @IntDef({DefaultValue.FlowAction.NONE, DefaultValue.FlowAction.PASS, DefaultValue.FlowAction.REJECT, DefaultValue.FlowAction.FINISH}) @Retention(RetentionPolicy.SOURCE) public @interface FlowActionAn { }

  • in model
    @FlowActionAn @InjectExtra("FlowAction") int mFlowAction;

  • in intent
    public FlowHandleActivity$$IntentBuilder.AfterSettingFlowAction FlowAction(@FlowActionAn int mFlowAction) { bundler.put("FlowAction", mFlowAction); return new FlowHandleActivity$$IntentBuilder.AfterSettingFlowAction(); }

@stephanenicolas
Copy link
Collaborator Author

@fansmc can we discuss the topic in a new issue. We are not against the idea, but we would like to understand / discuss things more in-depth. This will not be addressed in 3.0, but could be done quickly after. And even faster if you contribute a PR ;)

@stephanenicolas
Copy link
Collaborator Author

We also want to switch to gradle.

@stephanenicolas
Copy link
Collaborator Author

@f2prateek I am gonna merge a PR that will transform master into a new Dart 3 master.
The current version 2 master is now on branch version-2 in the repo if we want to patch it.

@stephanenicolas
Copy link
Collaborator Author

@dlemures @f2prateek There is a problem in the proposal above: we don't address (relatively rare) cases where 2 classes inherit from each other and live in different modules:

  • Module 1: Activity A with @BindExtra Foo foo; @BindExtra Bar bar;
  • Module 2: Activity B extends A

In this case, activity B's intent builder should include the field foo & bar in its DSL. But the problem is that there is no way for the current compiler to detect the fields of A. The compiler expects all classes to be compiled at once, and this will not be true anymore in the case of multiple modules (and also in the case of incremental compilation...).

We need to make this work, not only to solve the case of inheritance across module, but more importantly for the upcoming incremental annotation processing on Android.

On ButterKnife, this is still a work in progress, the idea of the current PR is to use the generated code for a super class, and actually use it, when processing a subclass in a different module. In Butterknife, the super class is used in 2 ways:

  1. as a marker that the super class had annotations and got processed by the annotation processor.
  2. the subclass's ViewBinder uses the super class ViewBinder.

The second point is a very nice feature, because it prevents from re-processing the super class, ButterKnife's annotation processor just uses the processing that was performed when the super class was compiled. Indeed, re-processing the super class when processing the subclass would be even more costly than just processing the super class (when compiling the super class): ButterKnife would even have to scan the annotations of the super class, whereas they are passed by javac to the annotation processor when compiling the super class itself.

We could use the same mechanism, but it's quite unlikely that we will be able to re-use the intent builder of the super class when generating the intent builder of the subclass. There are a few things that would break (namely 1) alphabetical ordering of the fields in the DSL, 2) the optional fields would have to be filled in a non natural order, and 3) there are some edge cases like when the subclass has no annotated fields of its own but only inherits annotated fields).

@stephanenicolas
Copy link
Collaborator Author

stephanenicolas commented Nov 16, 2017

Ideas have evolved a bit: Thx to @melix for our latest chat on source sets and compilation units.

We still have to figure out a solution for inheritance accross modules, but here is what a typical dart & henson 3 project will look like:
https://github.com/stephanenicolas/Henson_structure

This project illustrates how henson will work with respect to modularization:

  • module 1 exports an api of navigation that other modules can use to navigate to it
  • app uses this api to open an activity from module 1
  • module 1 doesn't know about app module, the same is true for app (except that app also plays the role of the aggregator module, and so, it knows module 1 here)
  • all the complexity of exporting a navigation api will be dealt with via a plugin
  • in module 1, the Foo uses a model : FooModel that lies in a different source tree ('src/main/navigation/java') this source set is compiled and jared and becomes the navigation api of module 1
  • another module can use this jar to create intents to go to Foo without having to know Foo.
  • For now there is a single sourceset for navigation models, but ideally they should take into accounts the variants of GAP

@stephanenicolas
Copy link
Collaborator Author

stephanenicolas commented Nov 17, 2017

Here is a repo that demonstrates a solution to the issue of inheritance across modules:
https://github.com/stephanenicolas/Henson_DSL_3

It should not be so hard to implement. This is basically using the same trick as ButterKnife to reuse the code generated for super classes. The main idea was to split the intent builder into 2 parts that can be merged easily (required fields by composition, optional fields by inheritance).

Thx to @code-twister for the exchanges on this.

@stephanenicolas
Copy link
Collaborator Author

There is now a more complete implementation of the DSL + a design document:
https://github.com/stephanenicolas/Henson_DSL_3/raw/master/assets/DSL-design.pdf

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