🪟 🔧 Move the serviceType prop out from Formik form (<ServiceForm/> refactoring part 2)

[dizel852]

What

Closes #18051
Part 1

Fixed use-case:

• user opens a create new Source/Destination
• choose the service type
• tries to leave the page

Previously:
Unsaved changes modal appears

Now:
The user didn't touch any field so there is no need to show the unsaved changes modal

How

Move the serviceType prop out from Formik form and handle it in <ConnectorCard /> component

Reading order(moving from bottom up)

• Connector/ServiceForm/types.ts:
  • added new type - ConnectorFormValues since we need to handle serviceType separately from ServiceFomValues
• Connector/ServiceForm/serviceFormContext.tsx:
  • clean up props: availableServices was passed in order to determinate what selectedService is. And since selectedServiceis also used on upper levels, it was replaced with selectedService
• Connector/ServiceForm/ServiceForm.tsx:
  • clean up props: we don't need availableServices since we don't pass it to context anymore. Was replaced with selectedService. testConnector - func argument was replaced with ConnectorFormValues, since ServiceFomValues doesn't contain serviceType anymore
  • <SetDefaultName /> - it was a tricky solution to set the default name for connector. Now we set default name in formValues on upper level (<ConnectorCard/ >)
  • move Doc Panel opening handling to the upper level, since it doesn't actually directly refers to <ServiceForm />
  • <ServiceForm /> became responsible for the actual form values
• Connector/ConnectorCard/ConnectorCard.tsx:
  • Previously ConnectorCardProps was extended by ServiceFormProps. But it was tricky to add the overwritten onSubmit handler. Since ServiceFormProps used onSubmit without serviceType, and ConnectorCardProps with. Simply Omit<ServiceFormProps, "onSubmit"> and overwrite didn't help and caused the errors. Left a TODO to fix that later on. And for now, just created the new ConnectorCardProps and marked which props used are in parent(<ConnectorCard />) and child(<ServiceFrom />) components:
  • 
  • Move Doc panel handling here
  • formValues - if isEdit we pass the existing values to <ServiceFrom /> otherwise set the service name for as the default connector name
• Update all relevant components after changes to all components which depend on ConnectorCard(DestinationForm.tsx, DestinationSettings.tsx, DestinationStep.tsx, SourceStep.tsx, SourceForm.tsx, SourceSettings.tsx)
• ...rest files...

[lmossman]

Thank you for the detailed PR description! I think all of the changes you described make sense, and I like that this simplifies ServiceForm even further and moves logic that is really the domain of the ConnectorCard into that component.

I just had one comment about getting rid of the term "Service" since we don't really use that term anywhere else. But, I think it could also make sense to make that change in a future PR if you don't want to increase the scope of this PR.

I tested this a bit locally and it seems to be working as expected from what I can tell. I also confirmed that selecting a connector type and then navigating away correctly does not show the warning modal 🎉

[lmossman]

While we're here changing things anyway, we should go ahead and try to get rid of the term "service" from all of these places.

It is confusing that this component and the ServiceForm component are the only places that refer to connectors and connector definitions as "services", so we should fix that. In this case, better names for these props would be onConnectorDefinitionSelect and availableConnectorDefinitions, to better match the terminology that we use for these objects throughout the rest of the Airbyte codebase.

Ideally we would also rename ServiceForm to something like ConnectorForm, and rename all related methods/variables/components as such.

[dizel852]

Also was thinking about that, but wasn't sure 😊
Ok, will fix that!

[dizel852]

Decided to try to rename all "service" related usages - got more modified files than I expected 😕
Good news - no "service" term mentions anymore 😊

[lmossman]

Could you create an issue for this, which captures why the current state is bad and what might be a better solution?

[dizel852]

Sure, here is it: #18553
I will add it to comment also

[timroes]

I'd suggest we move the default name logic into the ServiceForm where the initialValues of the Formik context are actually calculated, so that we have all the "formik" values logic ideally inside the ServiceForm as much as possible and not passing those values from the outside.

[dizel852]

Sounds reasonable, and I also did a try:
The first thing which confuses me, we just replace one prop(formValues) with another dependency - connector.
And here is the place where the errors come:

Because of:

interface ConnectorCardCreateProps extends ConnectorCardBaseProps {
  isEditMode?: false;
}

interface ConnectorCardEditProps extends ConnectorCardBaseProps {
  isEditMode: true;
  connector: ConnectorT;
}

export const ConnectorCard: React.FC<ConnectorCardCreateProps | ConnectorCardEditProps>

Also did a try to pass the connector prop implicitly to <ServiceForm /> just for testing, and surprisingly, the tests start failing...

Should I continue digging into it?
Since the PR already has some changes out of the scope.

[timroes]

There is still some code in https://github.com/airbytehq/airbyte/blob/master/airbyte-webapp/src/views/Connector/ServiceForm/useBuildForm.tsx#L33 when calculating the default values that has the serviceType still on it, which I believe we should be able to remove now as well.

I wonder if we might want to break the useBuildForm hook apart and remove the initialValues calculation from it? It feels the two calculation done in this hook have pretty much nothing to do with each other, so I feel they should either be two hooks or the initialValues could simply be inlined into the ServiceForm, since it's not really much or complex code at all?

[dizel852]

There is still some code in https://github.com/airbytehq/airbyte/blob/master/airbyte-webapp/src/views/Connector/ServiceForm/useBuildForm.tsx#L33 when calculating the default values that has the serviceType still on it, which I believe we should be able to remove now as well.

Missed that. Fixed

I wonder if we might want to break the useBuildForm hook apart and remove the initialValues calculation from it? It feels the two calculation done in this hook have pretty much nothing to do with each other, so I feel they should either be two hooks or the initialValues could simply be inlined into the ServiceForm, since it's not really much or complex code at all?

Agree, honesty I think the whole useBuildForm file should be refactored as well.
Can we make it in a separate PR?
This will make our refactoring more iterative.
WDYT?

[timroes]

Can we make it in a separate PR?

totally fine pushing this to another PR!

[dizel852]

totally fine pushing this to another PR!
@timroes need your final approval 😁

[timroes]

Code LGTM. Have done a quick smoke test locally, things seem to still work.