-
Notifications
You must be signed in to change notification settings - Fork 25
Lifecycle 2.0 with new design #376
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
base: next-bidirectional
Are you sure you want to change the base?
Conversation
src/openrpc/discovery.json
Outdated
"name": "getNavigationIntent", | ||
"tags": [ | ||
{ | ||
"name": "property:immutable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the immutable because there won't be any setter or event for this method.. Yes, I can change that
src/openrpc/lifecycle.json
Outdated
], | ||
"summary": "Listen to the start event", | ||
"params": [ | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the spacing here is off
src/schemas/intents.json
Outdated
{ | ||
"action": "preload", | ||
"context": { | ||
"source": "voice" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/openrpc/lifecycle.json
Outdated
], | ||
"components": { | ||
"schemas": { | ||
"LifecycleEvent": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/openrpc/lifecycle.json
Outdated
"description": "An object describing state of destroy with reason, and previous state", | ||
"type": "object", | ||
"required": [ | ||
"reason", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notifiers don't have a "listen" param, that's for the subscriber, onDestroy.
The parameter here should be the destroy reason.
src/openrpc/lifecycle.json
Outdated
} | ||
} | ||
], | ||
"result": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notifiers have no result, you should remove it entirely, which is in fact how OpenRPC knows it's a notification, not a method
src/openrpc/lifecycle.json
Outdated
"summary": "Listen to the start event", | ||
"params": [ | ||
{ | ||
"name": "listen", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notifiers don't have a listen
param, they just have the event payload(s). These events all have no payloads except for destroy
src/openrpc/lifecycle.json
Outdated
"result": { | ||
"name": "value", | ||
"schema": { | ||
"$ref": "#/components/schemas/LifecycleEvent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/schemas/intents.json
Outdated
] | ||
}, | ||
"PreLoadIntent": { | ||
"description": "A Firebolt compliant representation of a user intention to pre-load an app.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably remove the 'user intention' part as a user wouldn't be responsible for pre-loading an app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
] | ||
}, | ||
{ | ||
"name": "exclude-from-sdk" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
src/openrpc/lifecycle.json
Outdated
{ | ||
"name": "Default Example", | ||
"params": [], | ||
"result": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notifiers have no results.
src/openrpc/lifecycle.json
Outdated
"name": "Default Example", | ||
"params": [], | ||
"result": { | ||
"name": "Default Result", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notifiers have no result. Does this pass validation via firebolt-openrpc
? If so we should fix that.
src/openrpc/lifecycle.json
Outdated
"name": "Default Example", | ||
"params": [], | ||
"result": { | ||
"name": "Default Result", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notifiers have no results.
src/openrpc/lifecycle.json
Outdated
"name": "Default Example", | ||
"params": [], | ||
"result": { | ||
"name": "Default Result", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
src/openrpc/lifecycle.json
Outdated
{ | ||
"name": "Default Example", | ||
"params": [], | ||
"result": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
src/openrpc/lifecycle.json
Outdated
} | ||
], | ||
"result": { | ||
"name": "Default Result", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
di
src/schemas/intents.json
Outdated
] | ||
}, | ||
"PreLoadIntent": { | ||
"description": "A Firebolt compliant representation of a user intention to pre-load an app.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
export default { | ||
ready: function() { | ||
inactive.previous = 'initializing' | ||
paused.previous = 'initializing' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
No description provided.