PolymerLabs / arcs

Arcs
BSD 3-Clause "New" or "Revised" License
57 stars 35 forks source link

Implement eagerly instantiated recipes for Push scenario. #3823

Open piotrswigon opened 5 years ago

piotrswigon commented 5 years ago

Add a capability to instantiate "push" recipes eagerly, one arc per recipe.

This I reckon should be done by shell when it starts. The shell should iterate over all recipes in the context, find all marked as eagerly launched (see details below) and instantiate them.

Let's use @trigger annotation implemented for the SimplePlanner for this purpose. I propose a following marker:

@trigger
  launch startup
recipe
  (...)

If someone has a better idea please speak up.

Above annotation is present in the recipe in recipe.triggers as an array of arrays, so it show up as [['launch', 'startup']].

It is a fine idea to log a warning if we find a different value in a trigger. Something like: Unrecognized trigger value: ${key} ${value}.

We will use them at some later stage, but right now we will not.

piotrswigon commented 5 years ago

@sjmiles I presume this is quite doable? Let me know whether you could manage adding that sometime in the next few days :)

mariakleiner commented 5 years ago

another option (preferable imo) is for the Arcs service to run these recipes on startup. so far we've kept the pipes-shell agnostic of the android specific implementation details.

ps: the "quite doable" part still stands :)

piotrswigon commented 5 years ago

Does Arcs Service has an easy access to the context? Wouldn't your proposal require the Arcs Service to query the shell about what recipes are annotated for startup and then returning the same list back to the shell to run them?

yuangu-google commented 5 years ago

An alternative is to have another Service (ArcsCoreService) connect to ArcsService and instantiate these recipes. So you can separate the framework and the core components, which makes it more modular.

piotrswigon commented 5 years ago

What about keeping it simple? In the shell it's probably 10 lines of code.

I would rather argue having a URL param on the pipes shell dictating whether to do this or not. &launchStartupRecipes or something shorter.

piotrswigon commented 5 years ago

Also, if this is another service, it would need to be notified whenever a WebView gets reloaded, which a bugprone hassle.

yuangu-google commented 5 years ago

I'd wait for sjmiles@ for more comments on the simplicity. One question from me is that what if we port Arcs to other platforms? How will this @trigger launch startup work then?

And for the notification of WebView reloaded, basically you need to implement this mechanism for any user of Arcs that's not in the same Service. This is a fundamental drawback of wrapping Arcs in a WebView. Actually it's a fundamental problem of having Arcs as a framework. A framework can crash and the users of it need to know about this.

yuangu-google commented 5 years ago

In general I think if we can do things without changing the syntax/runtime/mechanism, we should do it this way, which could also be a great proof that our framework provides great extensibility.

mariakleiner commented 5 years ago

Today shell notifies the ArcsService when it's ready and sends it a list of recipes. We could update this call to include the entire manifest. Or just the list of triggers.

But also something to keep in mind is that right now the pipes-shell has a hardcoded manifest name which doesn't seem right. I think instead, the manifest file name should be provided by the ArcsService.

ATM the java code doesn't have equivalent of all recipes/*.ts classes and the manifest loading machinery, this can only be done in webview. But this problem should be solved as part of the RelEx anyway.

@yuangu-google for simplicity purposes I'd refrain from adding services until we have the post v0 architecture roughly figured out.

yuangu-google commented 5 years ago

If we need a short-term solution, I'd vote for doing this in pipes-shell directly. So we don't need to change ArcsService or add another keyword. Since pipes-shell is already doing hardcoded-things, I wouldn't mind doing another in it.

mariakleiner commented 5 years ago

In general it is the ArcsService job to start arcs and run recipes and it is a really simple solution to extend the "ready" message to include the trigger values, and it seems more correct in terms of separation of concerns.

yuangu-google commented 5 years ago

I asked sjmiles@'s to add recipes in the ready message for debugging purpose only. I didn't intend ArcsService to do any initialization of recipes.

So if we want to start these recipes at start up time, I prefer we just do it in pipes-shell than in ArcsService. Doing it in pipes-shell gives us the advantage of reloading this part without re-installing an apk nor re-importing the codes.