Misc Improvements to assist Connection Form Refactor #16303
Conversation
github-actions
bot
added
area/platform
| @@ -104,10 +91,7 @@ const CreateConnectionContent: React.FC<CreateConnectionContentProps> = ({ | |||
| const job = LogsRequestError.extractJobInfo(schemaErrorStatus); | |||
| return ( | |||
| <ContentCard> | |||
| <TryAfterErrorBlock | |||
| onClick={onDiscoverSchema} | |||
| additionControl={<SkipButton>{additionBottomControls}</SkipButton>} | |||
There was a problem hiding this comment.
I believe this was removed because there were never any additionBottomControls passed in, correct?
| await onDiscoverSchema(); | ||
| } | ||
| })(); | ||
| if (sourceId) { |
There was a problem hiding this comment.
async removed because there is no need to await the resolution of this
| @@ -21,7 +21,7 @@ interface DestinationFormProps { | |||
| afterSelectConnector?: () => void; | |||
| destinationDefinitions: DestinationDefinitionRead[]; | |||
| hasSuccess?: boolean; | |||
| error?: { message?: string; status?: number } | null; | |||
| error?: FormError | null; | |||
| status?: number; | ||
| } | ||
|
|
||
| export const createFormErrorMessage = (error: FormError): JSX.Element | string | null => { |
There was a problem hiding this comment.
Why is this specific to createForm? This looks more generic than that. I'm also not sure from the name if this means there was an error creating the form or an error within the creation form (for connections? for connectors?)
There was a problem hiding this comment.
Maybe it's for creating a form error message! Again, this seems more generic than forms? and maybe parseErrorMessage would be less ambiguous?
There was a problem hiding this comment.
generateMessageFromError
| fakeStatusError.status = 401; | ||
| expect(createFormErrorMessage(fakeStatusError)).toMatchInlineSnapshot(` | ||
| <Memo(MemoizedFormattedMessage) | ||
| id="form.someError" |
There was a problem hiding this comment.
Why do we want this returning some error rather than the actual error? We've run into a few places in the UI recently that we've undone places that we mask the actual error returned. Curious if that may apply here too.
There was a problem hiding this comment.
<FormattedMessage id="form.someError" />
It's the translation string.
There was a problem hiding this comment.
Right, but why do we want that instead of the actual error that is returned?
There was a problem hiding this comment.
This only occurs if the error does not have a message and returns a not-400 error.
I think it's just a "the server had a problem idk" type message since it didn't give us anything.
| interface ConnectionFormProps { | ||
| export type ConnectionOrPartialConnection = | ||
| | WebBackendConnectionRead | ||
| | (Partial<WebBackendConnectionRead> & Pick<WebBackendConnectionRead, "syncCatalog" | "source" | "destination">); |
There was a problem hiding this comment.
Can you help me understand this type? I'm trying to understand how we'd end up with a partial connection. Or is this in anticipation of the PATCH endpoints?
There was a problem hiding this comment.
When we create a connection we only pass in these properties.
|
|
||
| interface FormikConnectionFormValues { | ||
| export interface FormikConnectionFormValues { |
There was a problem hiding this comment.
It looks like this is just slightly different from the ValuesProps type. Would it make sense to have them reference each other somehow?
There was a problem hiding this comment.
I'm not sure it's worth trying to bridge the types. There's differences in what's optional.
| @@ -41,9 +42,9 @@ interface FormikConnectionFormValues { | |||
| normalization?: NormalizationType; | |||
| } | |||
|
|
|||
| type ConnectionFormValues = ValuesProps; | |||
| export type ConnectionFormValues = ValuesProps; | |||
There was a problem hiding this comment.
Looking through the code, I believe what happens is the FormikConnectionFormValues are what we use to interact with the form, then there's some casting/transforming and ConnectionFormValues is what is submitted to the API?
I think we should add a comment explaining that in here (or if I'm wrong, add a comment explaining the right thing :) )
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah. Just thinking if it took me a few minutes to figure that out, we could save future someones a few minutes each by commenting why there are two types for values.
There was a problem hiding this comment.
I have a TODO comment in the main PR about aligning the types. Might not be easy though.
There was a problem hiding this comment.
Gotcha. We can resolve this then since it sounds like there's some clarification in the other PR. Thanks!
* Move exports to declaration for formConfig * Add ConnectionOrPartialConnection, move ConnectionForm exports * Removing additionBottomControls as it was unused * Create and use FormError, test createFormErrorMessage * Remove unnecessary eslint ignore, remove unnecessary async/await * rename createFormErrorMessage
* Move exports to declaration for formConfig * Add ConnectionOrPartialConnection, move ConnectionForm exports * Removing additionBottomControls as it was unused * Create and use FormError, test createFormErrorMessage * Remove unnecessary eslint ignore, remove unnecessary async/await * rename createFormErrorMessage
What
During the Connection Form Refactor work there were various improvements I made as I encountered certain challengers with the refactor. I have consolidated some here which improves the code base and will make the Connection Form Refactor review cleaner.
How
Various typing and stylistic improvements.
One test added.
Removed a unused property.
Recommended reading order
Any
馃毃 User Impact 馃毃
I do not anticipate any issues. This would affect the Refresh Schema button. Other changes would break on build or test.