Consolidate oauth2 provider type #194
Conversation
Codecov Report
@@ Coverage Diff @@
## main #194 +/- ##
=======================================
Coverage 99.24% 99.24%
=======================================
Files 56 56
Lines 2125 2125
Branches 136 136
=======================================
Hits 2109 2109
Misses 15 15
Partials 1 1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| }; | ||
|
|
||
| /** | ||
| * TODO: The type system here could be improved one more then one provider type (CUSTOM) is available | ||
| * provider_name, authorization_url, token_url, identity_config and authorization_url_extras |
There was a problem hiding this comment.
In theory these should be required but I fear that making these fields required might break some apps
There was a problem hiding this comment.
How did you determine that these fields are required? Checked the backend schema or some other way?
Can you confirm that not providing these fields works? I.e. can you deploy an app and pass manifest validation if you remove these fields?
There was a problem hiding this comment.
The comments for each of those field mentioned that they we required for CUSTOM provider types, I am assuming they are true
But since those fields are currently not required I don't think we can make them required without potentially breaking apps
There was a problem hiding this comment.
"Breaking" from a static-type-check perspective, that is true, however, if the backend requires these fields anyways, then that is much less a concern, I think, because no real app would be able to define a provider without them.
That's why I think we should test this out on a real app 😄
| }; | ||
|
|
||
| /** | ||
| * TODO: The type system here could be improved one more then one provider type (CUSTOM) is available | ||
| * provider_name, authorization_url, token_url, identity_config and authorization_url_extras |
There was a problem hiding this comment.
How did you determine that these fields are required? Checked the backend schema or some other way?
Can you confirm that not providing these fields works? I.e. can you deploy an app and pass manifest validation if you remove these fields?
|
Converted this PR into |
Summary
This PR aims to address this comment that asks to consolidate
ManifestOAuth2ProviderSchemaandOAuth2ProviderDefinitionArgsI've left a TODO comment that mentiones that some work regarding the
typesystem could be done once more then one Provider types are present.testing
"deno-slack-sdk/"value with"https://raw.githubusercontent.com/slackapi/deno-slack-sdk/consolidate-Oauth2ProviderType/src/"Requirements