Lifecycle 2.0 with new design#376
Conversation
| "name": "getNavigationIntent", | ||
| "tags": [ | ||
| { | ||
| "name": "property:immutable" |
There was a problem hiding this comment.
is property:immutable necessary? Could it just be left off and treated as a basic getter?
The response can change so immutable might not be appropriate--I'd expect that to be used on truly immutable properties like a serial number.
There was a problem hiding this comment.
I added the immutable because there won't be any setter or event for this method.. Yes, I can change that
| ], | ||
| "summary": "Listen to the start event", | ||
| "params": [ | ||
| { |
There was a problem hiding this comment.
the spacing here is off
| { | ||
| "action": "preload", | ||
| "context": { | ||
| "source": "voice" |
There was a problem hiding this comment.
Minor detail, but "voice" is an odd example of a preload intent source. we have a value of "other" that is probably more appropriate for this use case.
| ], | ||
| "components": { | ||
| "schemas": { | ||
| "LifecycleEvent": { |
There was a problem hiding this comment.
This schema can go away. None of the Lifecycle events have parameters now, except for onDestroy.
We don't tell the app the "previous" and current state, because the event names are unique depending on where you came from.
| "description": "An object describing state of destroy with reason, and previous state", | ||
| "type": "object", | ||
| "required": [ | ||
| "reason", |
There was a problem hiding this comment.
We don't want previous here either.
| } | ||
| ], | ||
| "summary": "Listen to the destroy event \n\nHowever, this event is unreliable and app may not get this notification in the event of platform needs to kill the app quickly", | ||
| "params": [ |
There was a problem hiding this comment.
Notifiers don't have a "listen" param, that's for the subscriber, onDestroy.
The parameter here should be the destroy reason.
| } | ||
| } | ||
| ], | ||
| "result": { |
There was a problem hiding this comment.
Notifiers have no result, you should remove it entirely, which is in fact how OpenRPC knows it's a notification, not a method
| "summary": "Listen to the start event", | ||
| "params": [ | ||
| { | ||
| "name": "listen", |
There was a problem hiding this comment.
notifiers don't have a listen param, they just have the event payload(s). These events all have no payloads except for destroy
| "result": { | ||
| "name": "value", | ||
| "schema": { | ||
| "$ref": "#/components/schemas/LifecycleEvent" |
There was a problem hiding this comment.
Notifiers have no result, that's how OpenRPC knows that they're notifiers and not methods. You should remove the result from all of the notifiers.
| ] | ||
| }, | ||
| "PreLoadIntent": { | ||
| "description": "A Firebolt compliant representation of a user intention to pre-load an app.", |
There was a problem hiding this comment.
We can probably remove the 'user intention' part as a user wouldn't be responsible for pre-loading an app.
| ] | ||
| }, | ||
| { | ||
| "name": "exclude-from-sdk" |
There was a problem hiding this comment.
We need this to be in the C++ SDK, so I don't think this tag is correct, unless we want to say C++ includes all APIs even with this tag.
I was imagining something more like a list of capabilities to exclude from each language, somewhere inside the sdk.config.json, e.g.:
{
"languages": {
"javascript": {
"excludeModules": [
"Lifecycle"
]
}
}
}There was a problem hiding this comment.
Yea @kevinshahfws let's sync up on how resolve this and come up with a quick solution.
We'll probably need more granular, API-specific configs (e.g. the W3C media caps API should be excluded from the JS SDK, but the other MC APIs should still be there).
| { | ||
| "name": "Default Example", | ||
| "params": [], | ||
| "result": { |
There was a problem hiding this comment.
notifiers have no results.
| "name": "Default Example", | ||
| "params": [], | ||
| "result": { | ||
| "name": "Default Result", |
There was a problem hiding this comment.
notifiers have no result. Does this pass validation via firebolt-openrpc? If so we should fix that.
| "name": "Default Example", | ||
| "params": [], | ||
| "result": { | ||
| "name": "Default Result", |
There was a problem hiding this comment.
notifiers have no results.
| "name": "Default Example", | ||
| "params": [], | ||
| "result": { | ||
| "name": "Default Result", |
| { | ||
| "name": "Default Example", | ||
| "params": [], | ||
| "result": { |
| } | ||
| ], | ||
| "result": { | ||
| "name": "Default Result", |
| ] | ||
| }, | ||
| "PreLoadIntent": { | ||
| "description": "A Firebolt compliant representation of a user intention to pre-load an app.", |
| export default { | ||
| ready: function() { | ||
| inactive.previous = 'initializing' | ||
| paused.previous = 'initializing' |
There was a problem hiding this comment.
we need to remove all of these xxx.previous = lines from this class, as it's not part of the schema anymore.
There was a problem hiding this comment.
actually, this entire class can go away, as we don't have the module in JS any longer.
Not sure if that's a separate PR?
| throw 'Cannot call finished() except when in the unloading transition' | ||
| } | ||
| } | ||
| // function finished() { |
There was a problem hiding this comment.
we can get rid of this entire class, too, but not sure what your plan is for turning off Lifecycle in JS.
If you're keeping it around temporarily, why did you only drop finished?
3 participants
No description provided.