Skip to content

Fix dynamic props in the Connect demo app #14887

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Dec 9, 2024
Merged

Fix dynamic props in the Connect demo app #14887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec9dfa1
Fix dynamic props in the Connect demo app
jverce Dec 7, 2024
48c0f5b
Fix linter issues
jverce Dec 9, 2024
ef1cd33
Bump preview version
jverce Dec 9, 2024
df45457
Update PNPM lock file
jverce Dec 9, 2024
54eef81
Merge branch 'master' into fix/dynamic-props-in-demo-app
jverce Dec 9, 2024
e8f9720
Amend comment
jverce Dec 9, 2024
7949c2f
Update PNPM lock file
jverce Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/connect-react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@pipedream/connect-react",
"version": "1.0.0-preview.7",
"version": "1.0.0-preview.8",
"description": "Pipedream Connect library for React",
"files": [
"dist"
Expand Down
21 changes: 12 additions & 9 deletions packages/connect-react/src/hooks/form-context.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ import type {
import { useFrontendClient } from "./frontend-client-context";
import type { ComponentFormProps } from "../components/ComponentForm";

export type DynamicProps<T extends ConfigurableProps> = { id: string; configurable_props: T; }; // TODO
export type DynamicProps<T extends ConfigurableProps> = { id: string; configurableProps: T; }; // TODO

export type FormContext<T extends ConfigurableProps> = {
component: V1Component<T>;
configurableProps: T; // dynamicProps.configurable_props || props.component.configurable_props
configurableProps: T; // dynamicProps.configurableProps || props.component.configurable_props
configuredProps: ConfiguredProps<T>;
dynamicProps?: DynamicProps<T>; // lots of calls require dynamicProps?.id, so need to expose
dynamicPropsQueryIsFetching?: boolean;
Expand Down Expand Up @@ -117,11 +117,11 @@ export const FormContextProvider = <T extends ConfigurableProps>({
"dynamicProps",
],
queryFn: async () => {
const { dynamic_props } = await client.componentReloadProps(componentReloadPropsInput);
const { dynamicProps } = await client.componentReloadProps(componentReloadPropsInput);
// XXX what about if null?
// TODO observation errors, etc.
if (dynamic_props) {
setDynamicProps(dynamic_props);
if (dynamicProps) {
setDynamicProps(dynamicProps);
Comment on lines +120 to +124
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider improving the query function implementation.

The current implementation has a few concerns:

  1. The function mutates state and returns an empty array, which is an anti-pattern in React Query.
  2. There's no error handling for the case when dynamicProps is null.

Consider refactoring to:

- const { dynamicProps } = await client.componentReloadProps(componentReloadPropsInput);
- if (dynamicProps) {
-   setDynamicProps(dynamicProps);
- }
- setReloadPropIdx(undefined);
- return []; // XXX ok to mutate above and not look at data?
+ const response = await client.componentReloadProps(componentReloadPropsInput);
+ if (!response.dynamicProps) {
+   throw new Error('Dynamic props not found');
+ }
+ return response.dynamicProps;

Then handle the state updates in onSuccess callback:

useQuery({
  // ...
  onSuccess: (data) => {
    setDynamicProps(data);
    setReloadPropIdx(undefined);
  },
});

}
setReloadPropIdx(undefined);
return []; // XXX ok to mutate above and not look at data?
Expand All @@ -130,7 +130,7 @@ export const FormContextProvider = <T extends ConfigurableProps>({
});

// XXX fix types of dynamicProps, props.component so this type decl not needed
let configurableProps: T = dynamicProps?.configurable_props || formProps.component.configurable_props || [];
let configurableProps: T = dynamicProps?.configurableProps || formProps.component.configurable_props || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix inconsistent property naming and improve type safety.

The line still uses snake_case configurable_props from component prop, which is inconsistent with the camelCase convention being adopted.

- let configurableProps: T = dynamicProps?.configurableProps || formProps.component.configurable_props || [];
+ let configurableProps: T = dynamicProps?.configurableProps || formProps.component.configurableProps || [];

Also, consider adding runtime type checking for the assigned value since the type assertion to T might hide potential type mismatches.

Committable suggestion skipped: line range outside the PR's diff.

if (propNames?.length) {
const _configurableProps = [];
for (const prop of configurableProps) {
Expand Down Expand Up @@ -169,7 +169,7 @@ export const FormContextProvider = <T extends ConfigurableProps>({
errs.push("not a boolean");
}
} else if (prop.type === "string") {
if (typeof value !== "string" ) {
if (typeof value !== "string") {
errs.push("not a string");
}
} else if (prop.type === "app") {
Expand All @@ -179,7 +179,7 @@ export const FormContextProvider = <T extends ConfigurableProps>({
};

const updateConfiguredPropsQueryDisabledIdx = (configuredProps: ConfiguredProps<T>) => {
let _queryDisabledIdx = undefined;
let _queryDisabledIdx = undefined;
for (let idx = 0; idx < configurableProps.length; idx++) {
const prop = configurableProps[idx];
if (prop.hidden || (prop.optional && !optionalPropIsEnabled(prop))) {
Expand Down Expand Up @@ -241,7 +241,10 @@ export const FormContextProvider = <T extends ConfigurableProps>({
]);

// clear all props on user change
const [prevUserId, setPrevUserId] = useState(userId)
const [
prevUserId,
setPrevUserId,
] = useState(userId)
useEffect(() => {
if (prevUserId !== userId) {
updateConfiguredProps({});
Expand Down
33 changes: 17 additions & 16 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading