Add ConfigEditor util components #4

andresmgot · 2021-11-10T09:18:39Z

Ref: #28

In #28 I suggest to create a generic ConfigEditor component to use in the different data sources. After a first revision, I don't think that's the best path forward, it forces to create some strict structure of the configuration that may be difficult to update and understand.

Rather than exposing a ConfigEditor, I have created two wrapper components around the ResourceSelector and the Input components to adapt them to the ConfigEditor requirements.

sunker

Great stuff!

sunker · 2021-11-10T11:35:26Z

rollup.config.ts

@@ -26,7 +26,7 @@ const buildCjsPackage = ({ env }) => {
        },
      },
    ],
-    external: ['react', '@grafana/data', '@grafana/ui', 'lodash'],
+    external: ['react', '@grafana/data', '@grafana/ui', 'lodash', '@grafana/runtime'],


Can we avoid adding this dep?

The ConnectionConfig in this package is currently used in the DataSourceHttpSettings.tsx component in Grafana UI. This is because any data source that uses http for data source auth can theoretically use AWS SIGV4 to authenticate, given grafana is running on an EC2 instance or in AMG. This means grafana/ui depends on the grafana/aws-sdk. grafana/ui cannot, or at least should not, depend on grafana/runtime. I think in theory, this should be fine because it's only the ConnectionConfig that is used inside grafana/ui, but I seem to recall that all dependencies from the aws-sdk are being pulled into the grafana/ui build, and in that case this won't work. Might be because tree shaking is not setup correctly in the grafana ui package bundle, don't know.

Beside from the above mentioned problem, I also think it's better that the components that we expose in this package don't depend on the runtime. It should be up to the plugin to specify it's dependency to grafana. So it's better to just pass the getBackendSrv func as a prop to the config editor.

I would have never found that 😅 thanks for the insight!

sunker · 2021-11-10T11:48:28Z

src/sql/ConfigEditor/ConfigEditor.tsx

+    if (elem.type) {
+      return elem;
+    }
+    if ('fetch' in i) {


Use a ts guard here so you don't have to cast it with as

right, using the in operator, it's not needed to use as.

sunker · 2021-11-10T11:51:05Z

src/sql/ConfigEditor/ConfigEditor.tsx

+      return <Selector key={i.id} {...props} input={i as SelectorInput} saveOptions={saveOptions} />;
+    } else {
+      // The input is a text field
+      const input = i as TextInput;


src/sql/ResourceSelector.tsx

sarahzinger

Looks good to me! I had a few random thoughts as I read that I shared, but I don't think of any of them as blocking just things that occurred to me while reading

sarahzinger · 2021-11-10T15:53:41Z

src/sql/ConfigEditor/ConfigEditor.tsx

+import { ConnectionConfig } from 'ConnectionConfig';
+
+type SQLDataSourceJsonData<T extends AwsAuthDataSourceJsonData> = T & {
+  [n: string]: any;


random and unimportant but you could use Record here if you prefer

sarahzinger · 2021-11-10T16:20:06Z

src/sql/ConfigEditor/InlineText.tsx

+      <Input
+        data-testid={input['data-testid']}
+        className="width-30"
+        value={input.value || props.options.jsonData[input.id] || ''}


what would you think about choosing either input.value or props.options.jsonData[input.id] rather than doing one or the other?

Not super happy with this either. I needed both, this is for the case in which props.options.jsonData[input.id] is an object, for example. In that case, the user needs to set value: props.options.jsonData[input.id].foo but I didn't want to force all the callers to set the value if 95% of the cases it will be value: props.options.jsonData[input.id]. I am following that pattern in other places like the onChange above in line 33.

at the end I replaced the id property with a full path of the jsonData property. That solves this issue and avoid having to define a value and jsonData.

sarahzinger · 2021-11-10T16:25:17Z

src/sql/ConfigEditor/ConfigEditor.tsx

+
+export interface SQLConfigEditorProps
+  extends DataSourcePluginOptionsEditorProps<SQLDataSourceJsonData<{}>, AwsAuthDataSourceSecureJsonData> {
+  inputs: Array<SelectorInput | TextInput | JSX.Element>;


I wonder if it would be simpler to use props.children and always pass existing props and saveOptions to those children and let the consumer of SQLConfigEditor have control over what goes inside it and whether or not it uses saveOptions. Might make it more flexible for the future as well? But maybe too flexible? Idk. Just sharing a thought in case it's helpful

right, I am not super fond of this solution. The thing is that I wanted to avoid the burden of having to generate a full ResourceSelector for the data sources, just setting a subset of properties but it's true that this array may not be the best. I will try to create a wrapper component and just use those as children 🤔

At the end I created a wrapper around the ResourceSelector applied to the configuration view and remove the ConfigEditor component. Now the caller needs to define the function saveOptions (and that will be duplicated) but it feels cleaner than this array.

cool seems sensible to me! If you find yourself not wanting saveOptions duplicated this package could export a hook useSaveOptions or something like that but also it makes sense to me that the individual packages would have that logic.

sarahzinger

I like the changes you made! I think it would be cool to have documentation somewhere of how to use these utils to make a ConfigEditor or something but that could happen in another pr maybe, perhaps when they are used we could just link to it being used somewhere

sunker

Nice! Just a nit

Yeah would be nice to see how this is used. Can you put up a draft PR for how redshift or athena use these components?

sunker · 2021-11-15T06:47:12Z

src/sql/ConfigEditor/InlineInput.test.tsx

+  jsonDataPath: 'foo',
+};
+
+describe('InlineText', () => {


InlineInput

sunker · 2021-11-15T06:49:57Z

src/sql/ConfigEditor/ConfigSelect.tsx

+  fetch: () => Promise<Array<string | SelectableValue<string>>>;
+  dependencies?: string[];
+  label?: string;
+  'data-testid'?: string;


Do you need the test id? Wouldn't using getByLabelTest suffice?

It doesn't, it's an issue with grafana/ui (I guess I should create an issue for this?). You get this error using getByLabelText:

TestingLibraryElementError: Found a label with the text of: <>, however no form control was found associated to that label. Make sure you're using the "for" attribute or "aria-labelledby" attribute correctly.

Also, the test-id is used in the e2e test (the flows only work with aria-labels and test-ids)

andresmgot · 2021-11-15T10:15:30Z

Thanks @sunker @sarahzinger! For reference, I have pushed some drafts for you to take a look to more complex examples (apart from the tests):

ConfigEditor

Add ConfigEditor Component

a1347f7

andresmgot requested review from sunker and sarahzinger November 10, 2021 09:18

andresmgot changed the title ~~Add ConfigEditor Component~~ Add SQLConfigEditor Component Nov 10, 2021

sunker reviewed Nov 10, 2021

View reviewed changes

sarahzinger approved these changes Nov 10, 2021

View reviewed changes

Apply feedback

88e8ddf

andresmgot changed the title ~~Add SQLConfigEditor Component~~ Add ConfigEditor util components Nov 11, 2021

andresmgot added 2 commits November 11, 2021 16:46

update title

4f71ded

use dependencies directly

58ca9f8

andresmgot mentioned this pull request Nov 12, 2021

Add QueryEditor components #5

Merged

1 task

andresmgot requested review from sarahzinger and sunker November 12, 2021 16:08

sarahzinger approved these changes Nov 15, 2021

View reviewed changes

sunker approved these changes Nov 15, 2021

View reviewed changes

review

25c9c2f

andresmgot merged commit 0e03616 into main Nov 15, 2021

andresmgot deleted the configEditor branch November 15, 2021 10:20

andresmgot mentioned this pull request Dec 2, 2021

Minor refactor of ConfigSelect #6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ConfigEditor util components #4

Add ConfigEditor util components #4

andresmgot commented Nov 10, 2021 •

edited

sunker left a comment

sunker Nov 10, 2021

andresmgot Nov 10, 2021

sunker Nov 10, 2021

andresmgot Nov 10, 2021

sunker Nov 10, 2021

sarahzinger left a comment

sarahzinger Nov 10, 2021

sarahzinger Nov 10, 2021

andresmgot Nov 11, 2021

andresmgot Nov 11, 2021

sarahzinger Nov 10, 2021

andresmgot Nov 11, 2021

andresmgot Nov 11, 2021

sarahzinger Nov 15, 2021

sarahzinger left a comment

sunker left a comment

sunker Nov 15, 2021

sunker Nov 15, 2021

andresmgot Nov 15, 2021

andresmgot commented Nov 15, 2021 •

edited

Add ConfigEditor util components #4

Add ConfigEditor util components #4

Conversation

andresmgot commented Nov 10, 2021 • edited

sunker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahzinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahzinger left a comment

Choose a reason for hiding this comment

sunker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented Nov 15, 2021 • edited

andresmgot commented Nov 10, 2021 •

edited

andresmgot commented Nov 15, 2021 •

edited