SWC-6662 - OpenAPI Client Generation for Synapse REST API #689
Conversation
nickgros
force-pushed
the
SWC-6662
branch
3 times, most recently
from
70d3e23
to
ed2dcd8
Compare
| @@ -0,0 +1 @@ | |||
| src/generated/ | |||
There was a problem hiding this comment.
Don't commit the generated models to git
There was a problem hiding this comment.
Integrated into synapse-react-client as an example -- needed to pick models/services that had a low code surface, because our hand-written models are incompatible with the generated models. The OAuth client services seemed to be ok for that
| client?.redirect_uris?.map((str: string) => ({ uri: str })) ?? [ | ||
| { uri: '' }, | ||
| ], |
There was a problem hiding this comment.
Note that redirect_uris is NOT required in the spec, so we have to do additional null handling or assertions to work with the new model. This will be common, and is IMO the biggest downside and reason to not use the generated code right now.
| (client?.userinfo_signed_response_alg ?? | ||
| defaultUserInfoSignedResponseAlgorithm) as | ||
| | 'JSON' | ||
| | OIDCSigningAlgorithm, |
There was a problem hiding this comment.
Bug where the property userinfo_signed_response_alg has the wrong type. The bug is in the spec, see PLFM-8319
| oAuthClient: oAuthClient, | ||
| }) | ||
| .then(precheckResult => { | ||
| setWarnTrigger(precheckResult.reverificationRequired!) |
There was a problem hiding this comment.
reverificationRequired is another property that is never null, but because the spec doesn't indicate it as required, we have to add the ! non-null assertion
| setWarnTrigger(precheckResult.reverificationRequired) | ||
| }) | ||
| synapseClient.openIDConnectServicesClient | ||
| .putAuthV1Oauth2ClientIdVerificationPrecheck({ |
There was a problem hiding this comment.
These auto-generated method names are not great. Should we create our own wrapper? Push for the ability to incrementally curate these in the backend?
| @@ -50,6 +54,16 @@ export function SynapseContextProvider(props: SynapseContextProviderProps) { | |||
| [providedContext.accessToken], | |||
| ) | |||
|
|
|||
| const synapseApiClient = useMemo(() => { | |||
| const configuration = new Configuration({ | |||
| fetchApi: fetchWithExponentialTimeout, | |||
There was a problem hiding this comment.
Super fortunate that our existing abstraction just plugs in here. We pass in our fetchWithExponentialTimeout and we maintain the same behavior that we already have with automatic retries and all errors converted to our custom SynapseClientError class.
It should probably the the default behavior for our synapse-client package, but that refactoring can wait (hence the TODO)
| ) | ||
| setTosUri(client?.tos_uri ?? '') | ||
| }, [isShowingModal, client]) | ||
|
|
||
| useDebouncedEffect( | ||
| () => { | ||
| if (accessToken && oAuthClient.client_id) { | ||
| if (oAuthClient.client_id) { |
There was a problem hiding this comment.
I don't think you can ever see this component without being logged in, so I'm not worried about the accessToken checks
nickgros
changed the title
|
Current approach will increase our IIFE bundle size from 3MB to 5MB - this is already very large, so need to rethink the approach |
nickgros
changed the title
…dd ConfigBuilder & use in all apps
No description provided.