Connection Form Refactor - Connection Form Service #15893
Conversation
…ion-form-refactor
…ion-form-refactor
…ion-form-refactor
github-actions
bot
added
area/platform
| onFrequencySelect, | ||
| onCancel, |
There was a problem hiding this comment.
These are in here to prevent prop-drilling.
…ion-form-refactor
krishnaglick
force-pushed
the
kc/connection-form-refactor
branch
from
9869327 to
bf6a647
Compare
|
Looks like the field for Connection Name input disappeared during connection creation. |
Should be fixed! |
airbyte-webapp/src/components/CreateConnectionContent/CreateConnectionContent.tsx
Outdated
Show resolved
Hide resolved
| const changedForms = useChangedFormsById(); | ||
| const formDirty = useMemo(() => !!changedForms?.[0]?.[formId], [changedForms, formId]); |
There was a problem hiding this comment.
Good alternative to tracking the state of the form without all that extra stuff!
Would this be cleaner? The useChangedFormsById behaves similar to a useState:
| const changedForms = useChangedFormsById(); | |
| const formDirty = useMemo(() => !!changedForms?.[0]?.[formId], [changedForms, formId]); | |
| const [changedFormsById] = useChangedFormsById(); | |
| const formDirty = useMemo(() => !!changedFormsById?.[formId], [changedForms, formId]); |
| // We only want to go to the new connection if we _do not_ have an after submit action. | ||
| if (!afterSubmitConnection) { | ||
| // We have to clear the form change to prevent the dirty-form tracking modal from appearing. | ||
| clearFormChange(formId); |
There was a problem hiding this comment.
This looks too high in the component tree and should be called from the Connection form. The reason is that the ConnectionForm sets up the ConnectionChangeTracker, and should be responsible for clearing it after submission. In this case, this component, which has no idea what the form does, assumes that the tracker is being used.
There was a problem hiding this comment.
The next PR brings the FormikContext "out" a layer at which point this would be the right place for this call. Does it seem reasonable to leave this for now knowing it will be much more readily adressable in the next PR?
There was a problem hiding this comment.
This will be changed in part 2.
| return { | ||
| submitCancelled: true, | ||
| }; | ||
| throw new ModalCancel(); |
There was a problem hiding this comment.
Throwing should only be used when there is a misuse of the function or an error. In this case, this is a valid action that the user can take. This forces the developer using to catch errors and remember to check that one of them is actually a cancel action.
|
@krishnaglick I'm getting a build error when running this locally now, same one as in CI: https://github.com/airbytehq/airbyte/actions/runs/3047952989/jobs/4912510015#step:7:144 |
| const formId = useUniqueFormId(); | ||
| const { clearFormChange } = useFormChangeTrackerService(); | ||
| const [changedFormsById] = useChangedFormsById(); | ||
| const formDirty = useMemo(() => !!changedFormsById?.[formId], [changedFormsById, formId]); |
There was a problem hiding this comment.
This all will get pulled out in part 2.
| if (!(e instanceof ModalCancel)) { | ||
| setSubmitError(e); | ||
| } |
There was a problem hiding this comment.
This will be changed in part 2.
| return { | ||
| submitCancelled: true, | ||
| }; | ||
| throw new ModalCancel(); |
There was a problem hiding this comment.
This will be changed in part 2.
| connection.syncCatalog.streams, | ||
| mode, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Declared at the appropriate level now.
…ion-form-refactor
|
Thanks for the all-clear Edmundo! |
* Work started * Minor cleanup * Some cleanup * Lots moved into context * WIP, stepping in the right direction * WIP testing * Post-merge fix * Observables! * WIP tests * Tests! * CI test * CI? * perhaps * Only show name field during create * Fix Build * Fix build * Fixing a bug * Fix failing test * Fixes e2e * Type consolidation * useCallback, improvements to connection create onAfter, and removing dirty tracking * cleanup * Removing an unused prop * errorStatusMessage and mapFormPropsToOperation tests * useUniqueFormId and useInitialValues tests * Cleanup, onFrequencySelect is moved to its use location, better test wrapper, and expanding use of FormError * Better formSubmit handling for new connection * Commenting and some cleanup * Comments! * Fixing errors from the merge * mock data cleanup * Better TODO * onFrequencySelect is now always called * Edmundo CR * Remove whitespace * Bridging changes to bring things inline * Builds and tests run Co-authored-by: Lake Mossman <lake@airbyte.io>
* Work started * Minor cleanup * Some cleanup * Lots moved into context * WIP, stepping in the right direction * WIP testing * Post-merge fix * Observables! * WIP tests * Tests! * CI test * CI? * perhaps * Only show name field during create * Fix Build * Fix build * Fixing a bug * Fix failing test * Fixes e2e * Type consolidation * useCallback, improvements to connection create onAfter, and removing dirty tracking * cleanup * Removing an unused prop * errorStatusMessage and mapFormPropsToOperation tests * useUniqueFormId and useInitialValues tests * Cleanup, onFrequencySelect is moved to its use location, better test wrapper, and expanding use of FormError * Better formSubmit handling for new connection * Commenting and some cleanup * Comments! * Fixing errors from the merge * mock data cleanup * Better TODO * onFrequencySelect is now always called * Edmundo CR * Remove whitespace * Bridging changes to bring things inline * Builds and tests run Co-authored-by: Lake Mossman <lake@airbyte.io>
Closes #15603
What
The Connection Create/Edit code is a dense and complex jungle. This is first portion of work to simplify it, and increase maintainability.
#16293 and #16303 spun out of this work as ways to improve the reliability and readability of existing code.
Recommended reading order
Start with:
There was a lot of logic wrapped up in the ConnectionForm. It contains the Formik form, handled passing the dirty state up VIA an ugly chain of logic, coordinated the submit & afterSubmit logic, and a few other misc things. Plus it handled various bits of prop drilling. A lot of this was moved into the ConnectionFormService which keeps us from prop drilling, and separates helps keep the render logic separate.
Then:
These two components will likely see greater change in the next PR in this series. There was a lot of back-and-forth with the ConnectionForm:
onSelectFrequencyanalytics callafterSubmitin what I felt was an awkward way. This has been streamlined.There were a few other changes, like the onSubmit returning
{ submitCancelled: true }now utilizes its try/catch, and both components get to do less prop drilling.After #16293 and #16303 are merged this diff will look cleaner, but there's various components like
StreamHeader.tsxwhere instead of getting a value from its props, it's just pulling it out of the context.Lastly there's some testing that's worth looking over!
One final note is
OperationsSection.tsxhad a Wrapper prop passed in. That's been cleaned up to just import the appropriate wrapping components.🚨 User Impact 🚨
This work touches the major pieces of our Connection Create and Edit pages. They're quite touchy and core to the application so this potentially has a large user impact if bugs are introduced so please test carefully!