Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🪟 🐛 Remount connector form on type switch #19511

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Nov 16, 2022

What

This fixes an issue that (timing based) the OAuth button sometims would not render and instead it would render out the raw fields. This time (yeah I know we "fixed" this already two weeks ago) it happened way less reliable.

How

Thanks to the refactorings we've done to the ConnectorForm, we no longer track the actual connector type as part of Formik. The ConnectorForm component itself though would still stay the same whenever you switch the connector type (because that's how React works), and just Formik inside that component would reinitialize.

This still kept a lot of state inside the ConnectorForm component around while changing the connector type. None of that state should actually be kept around when changing connector types. So we added a key to the ConnectorForm where we render it, that depends on the connector type, which causes React to fully unmount that component and mount a new instance of that component whenever the connector type changes. That way we can be sure no state is kept when switching connector types.

With that change we could no longer get into the state where OAuth fields where rendered out raw... and we REALLY tried.

Bonus

Also makes sure the examples in the connector form are always strings, since atm if a connector has an example of false it would just not render out, because React doesn't render false. Making sure this will now be "false".

@timroes timroes requested a review from a team as a code owner November 16, 2022 23:23
@octavia-squidington-iv octavia-squidington-iv added the area/platform issues related to the platform label Nov 16, 2022
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM. Did not test locally but I did witness Tim testing it over zoom, and it was working as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform team/extensibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants