-
Notifications
You must be signed in to change notification settings - Fork 119
UrlParams: Intend system update, split into configuration and propreties #3376
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: livekit
Are you sure you want to change the base?
Conversation
// behavior to the needs of specific consumers. | ||
export interface UrlParams { | ||
/** | ||
* This UrlProperties contains as the properties that are used by to pass required data to the widget. |
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 UrlProperties contains as the properties that are used by to pass required data to the widget. | |
* This UrlProperties contains the properties that are used by applications to pass required data to the widget. |
* and configure the behavior of the app. There value is the same if EC is used in | ||
* the same context but different accoutns/users. |
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.
* and configure the behavior of the app. There value is the same if EC is used in | |
* the same context but different accoutns/users. | |
* and configure the behavior of the app. Their value is the same if EC is used in | |
* the same context but with different accounts/users. |
type: { [s: string]: T } | ArrayLike<T>, | ||
): T | undefined { | ||
const value = this.getParam(name); | ||
if (value && Object.values(type).includes(value as T)) { |
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.
if (value && Object.values(type).includes(value as T)) { | |
if (value !== null && Object.values(type).includes(value as T)) { |
To avoid surprises with falsy strings
...properties, | ||
...intentPreset, | ||
...pickBy(configuration, (v) => v !== undefined), | ||
intent, |
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.
What do you think about the idea of not including intent
in the returned object here? That would enforce that we never predicate any behavior on the intent alone, and always provide some other URL parameter to override the behavior with.
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.
let me check it was used for. But your explanation for why it is not needed (should not be needed) sounds very sane to me.
default: | ||
intentPreset = { | ||
confineToRoom: false, | ||
appPrompt: false, |
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 think this should still be true
.
Needed for implementing #3372 more cleanly
This PR updates the intend system. We want to solve this before starting on the telephone usecase implementation to prohibit us needing to make too many rust-sdk PR's and to converge to a solution we think brings the best benefits to maintain the url paramters.
This solution tries to get us the most features from this list:
By implementing intends with a "preset" architecture:
platform
This PR changes:
UrlProperties
andUrlConfiguration
(configuration are the ones where you can skip with one intend, properties are always required)