-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: Clearer language around new feature status of URL elicitation #1825
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
fix: Clearer language around new feature status of URL elicitation #1825
Conversation
|
lgtm! |
|
Curious how people feel about this change from a developer toil standpoint. What this means in practice is that when a server creates the elicitation, it needs to now provide an additional flag on whether it can notify on completion or not. To summarize the change, if a server declares the elicitation as notification-ready, the client has to provide the affordance for the URI and then wait until the server lets it know that it's ready to proceed, and if the elicitation is not marked as notification-ready, then the client can either proceed as-is or wait a reasonable amount of time before continuing. Two angles here - client now needs to be flag-aware in the scenario, and server developers now need to mark their elicitations as notification-ready or not and then determine the course of action. Seems like that adds a bit of complexity. This feels like quite a bit of decision overhead. I wonder if we can effectively make the notification the default behavior, and only have some kind of manual completion flag that is set up if the server is not sending a notification, because having the notification seems like the right starter developer experience? @connor4312 @wdawson @nbarbettini - thoughts? |
Slight detour to be extra pedantic (it is a spec, after all 😉): the client doesn't have to wait -- up to the client whether waiting makes sense. So in all cases, the client could proceed as-is or wait a reasonable amount of time. Do you think that's clear enough in the current language? I agree in spirit with the idea of encouraging the notification as default behavior. I'm a little worried on the server-side that we'd be assuming that most of the URL elicitations folks want to do are completion-aware. Flows with callbacks like OAuth and some payment flows are, but the spec intentionally does not restrict URL elicitation to only those flows. |
| </Note> | ||
|
|
||
| URL mode elicitation requests **MUST** specify `mode: "url"` and include these additional parameters: | ||
| URL mode elicitation requests **MUST** specify `mode: "url"`, a `message`, and include these additional parameters: |
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.
No substantive change here, just clarifying that message (described earlier in the document) is still a required field for this type of elicitation/create request.
| | `elicitationId` | string | A unique identifier for the elicitation. | | ||
|
|
||
| The `url` parameter **MUST** contain a valid URL. The `message` parameter **MUST NOT** contain a URL. | ||
| The `url` parameter **MUST** contain a valid URL. |
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 removed the **MUST NOT** contain language elsewhere in the doc in #887 elsewhere, but I missed cleaning it up on this line.
localden
left a comment
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.
Just a minor nit, otherwise LGTM.
Co-authored-by: Den Delimarsky <53200638+localden@users.noreply.github.com>
|
While having a hint would be nice, this clarification makes sense and clears up ambiguities for me 🙂 |
|
|
||
| The `url` parameter **MUST** contain a valid URL. | ||
|
|
||
| <Note> |
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.
Shifted this <Note> down below the request params table, so that we don't have two <Note>s stacked on top of each other (looks weird visually). No change to the wording of either section.
Co-authored-by: Den Delimarsky <53200638+localden@users.noreply.github.com>
Motivation and Context
Addresses client implementor feedback in #1819
How Has This Been Tested?
n/a
Breaking Changes
No, this is an additive change to a RC capability.
Types of changes
Checklist