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

Add ConfigEditor util components #4

Merged
merged 5 commits into from
Nov 15, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"@babel/preset-react": "^7.13.13",
"@babel/preset-typescript": "^7.13.0",
"@grafana/data": "^8.2.1",
"@grafana/runtime": "^8.2.1",
"@grafana/toolkit": "^8.2.1",
"@grafana/tsconfig": "^1.0.0-rc1",
"@grafana/ui": "^8.2.1",
Expand Down
2 changes: 1 addition & 1 deletion rollup.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const buildCjsPackage = ({ env }) => {
},
},
],
external: ['react', '@grafana/data', '@grafana/ui', 'lodash'],
external: ['react', '@grafana/data', '@grafana/ui', 'lodash', '@grafana/runtime'],
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

plugins: [
typescript({
rollupCommonJSResolveHack: false,
Expand Down
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export { ConnectionConfig, ConnectionConfigProps } from './ConnectionConfig';
export { SQLConfigEditor, SQLConfigEditorProps, SelectorInput, TextInput } from './sql/ConfigEditor';
export { ResourceSelector, ResourceSelectorProps } from './sql/ResourceSelector';
export * from './types';
export * from './regions';
Expand Down
48 changes: 48 additions & 0 deletions src/sql/ConfigEditor/ConfigEditor.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import { SQLConfigEditor } from './ConfigEditor';
import { mockDatasourceOptions } from './__mocks__/datasource';
import { SelectorInput } from './Selector';
import { TextInput } from './InlineText';
import { AwsAuthType } from '../../types';

const props = {
...mockDatasourceOptions,
inputs: [] as Array<SelectorInput | TextInput | JSX.Element>,
};

const resetWindow = () => {
(window as any).grafanaBootData = {
settings: {
awsAllowedAuthProviders: [AwsAuthType.EC2IAMRole, AwsAuthType.Keys],
awsAssumeRoleEnabled: false,
},
};
};

describe('SQLConfigEditor', () => {
beforeEach(() => resetWindow());
afterEach(() => resetWindow());

it('should render a custom component', () => {
render(<SQLConfigEditor {...props} inputs={[<div key="hello">hello!</div>]} />);
expect(screen.queryByText('hello!')).toBeInTheDocument();
});

it('should render a selector', () => {
const input: SelectorInput = {
id: 'foo',
fetch: jest.fn(),
};
render(<SQLConfigEditor {...props} options={{ ...props.options, jsonData: { foo: 'bar' } }} inputs={[input]} />);
expect(screen.queryByText('bar')).toBeInTheDocument();
});

it('should render a text input', () => {
const input: TextInput = {
id: 'foo',
};
render(<SQLConfigEditor {...props} options={{ ...props.options, jsonData: { foo: 'bar' } }} inputs={[input]} />);
expect(screen.queryByDisplayValue('bar')).toBeInTheDocument();
});
});
63 changes: 63 additions & 0 deletions src/sql/ConfigEditor/ConfigEditor.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import React, { useState } from 'react';
import { DataSourcePluginOptionsEditorProps, DataSourceSettings } from '@grafana/data';
import { AwsAuthDataSourceJsonData, AwsAuthDataSourceSecureJsonData, AwsAuthDataSourceSettings } from '../../types';
import { getBackendSrv } from '@grafana/runtime';
import { Selector, SelectorInput } from './Selector';
import { InlineText, TextInput } from './InlineText';
import { ConnectionConfig } from 'ConnectionConfig';

type SQLDataSourceJsonData<T extends AwsAuthDataSourceJsonData> = T & {
[n: string]: any;
Copy link
Member

Choose a reason for hiding this comment

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

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

};

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

export function SQLConfigEditor(props: SQLConfigEditorProps) {
const baseURL = `/api/datasources/${props.options.id}`;
const [saved, setSaved] = useState(!!props.options.jsonData.defaultRegion);
const saveOptions = async () => {
if (saved) {
return;
}
await getBackendSrv()
.put(baseURL, props.options)
.then((result: { datasource: AwsAuthDataSourceSettings }) => {
props.onOptionsChange({
...props.options,
version: result.datasource.version,
});
});
setSaved(true);
};

const inputElements = props.inputs.map((i) => {
const elem = i as JSX.Element;
if (elem.type) {
return elem;
}
if ('fetch' in i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Here too

return <InlineText key={input.id} {...props} input={input} />;
}
});

const onOptionsChange = (options: DataSourceSettings<AwsAuthDataSourceJsonData, AwsAuthDataSourceSecureJsonData>) => {
setSaved(false);
props.onOptionsChange(options);
};

return (
<div className="gf-form-group">
<ConnectionConfig {...props} onOptionsChange={onOptionsChange} />
<h3>Data Source Details</h3>
{inputElements}
</div>
);
}
56 changes: 56 additions & 0 deletions src/sql/ConfigEditor/InlineText.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import React from 'react';
import { render, screen, fireEvent } from '@testing-library/react';
import { InlineText, TextInput } from './InlineText';
import { mockDatasourceOptions } from './__mocks__/datasource';

const props = {
...mockDatasourceOptions,
};

describe('SQLTextInput', () => {
it('should show jsonData value', () => {
const input: TextInput = {
id: 'foo',
};
render(<InlineText {...props} options={{ ...props.options, jsonData: { foo: 'bar' } }} input={input} />);
expect(screen.queryByDisplayValue('bar')).toBeInTheDocument();
});

it('should show a custom value', () => {
const input: TextInput = {
id: 'foo',
value: 'foobar',
};
render(<InlineText {...props} input={input} />);
expect(screen.queryByDisplayValue('foobar')).toBeInTheDocument();
});

it('should update jsonData', () => {
const input: TextInput = {
id: 'foo',
'data-testid': 'foo-id',
};
const onOptionsChange = jest.fn();
render(<InlineText {...props} input={input} onOptionsChange={onOptionsChange} />);
fireEvent.change(screen.getByTestId('foo-id'), { target: { value: 'bar' } });
expect(onOptionsChange).toHaveBeenCalledWith({
...props.options,
jsonData: {
...props.options.jsonData,
foo: 'bar',
},
});
});

it('should call custom onChange', () => {
const onChange = jest.fn();
const input: TextInput = {
id: 'foo',
'data-testid': 'foo-id',
onChange,
};
render(<InlineText {...props} input={input} />);
fireEvent.change(screen.getByTestId('foo-id'), { target: { value: 'bar' } });
expect(onChange).toHaveBeenCalled();
});
});
63 changes: 63 additions & 0 deletions src/sql/ConfigEditor/InlineText.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import React from 'react';
import { DataSourcePluginOptionsEditorProps, SelectableValue } from '@grafana/data';
import { AwsAuthDataSourceJsonData, AwsAuthDataSourceSecureJsonData } from '../../types';
import { InlineField, Input } from '@grafana/ui';
import { FormEvent } from 'react-dom/node_modules/@types/react';

export type TextInput = {
id: string;
label?: string;
tooltip?: string;
placeholder?: string;
'data-testid'?: string;
hidden?: boolean;
disabled?: boolean;
value?: string;
onChange?: (e: SelectableValue<string>) => void;
};

type SQLDataSourceJsonData<T extends AwsAuthDataSourceJsonData> = T & {
[n: string]: any;
};

export interface TextProps
extends DataSourcePluginOptionsEditorProps<SQLDataSourceJsonData<{}>, AwsAuthDataSourceSecureJsonData> {
input: TextInput;
}

export function InlineText(props: TextProps) {
const { input } = props;

const onChange = (e: FormEvent<HTMLInputElement>) => {
if (input.onChange) {
input.onChange(e);
} else {
props.onOptionsChange({
...props.options,
jsonData: {
...props.options.jsonData,
[input.id]: e.currentTarget.value || '',
},
});
}
};
return (
<InlineField
label={input.label}
labelWidth={28}
tooltip={input.tooltip}
key={input.id}
hidden={input.hidden}
disabled={input.disabled}
>
<Input
data-testid={input['data-testid']}
className="width-30"
value={input.value || props.options.jsonData[input.id] || ''}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

onChange={onChange}
placeholder={input.placeholder}
disabled={input.disabled}
/>
</InlineField>
);
}
69 changes: 69 additions & 0 deletions src/sql/ConfigEditor/Selector.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import { Selector, SelectorInput } from './Selector';
import { mockDatasourceOptions } from './__mocks__/datasource';
import { select } from 'react-select-event';

const props = {
...mockDatasourceOptions,
saveOptions: jest.fn(),
};

describe('SQLTextInput', () => {
it('should show jsonData value', () => {
const input: SelectorInput = {
id: 'foo',
fetch: jest.fn(),
};
render(<Selector {...props} options={{ ...props.options, jsonData: { foo: 'bar' } }} input={input} />);
expect(screen.queryByText('bar')).toBeInTheDocument();
});

it('should show a custom value', () => {
const input: SelectorInput = {
id: 'foo',
value: 'foobar',
fetch: jest.fn(),
};
render(<Selector {...props} input={input} />);
expect(screen.queryByText('foobar')).toBeInTheDocument();
});

it('should update jsonData', async () => {
const input: SelectorInput = {
id: 'foo',
label: 'foo-id',
fetch: jest.fn().mockResolvedValue(['bar']),
};
const onOptionsChange = jest.fn();
render(<Selector {...props} input={input} onOptionsChange={onOptionsChange} />);

const selectEl = screen.getByLabelText(input.label);
expect(selectEl).toBeInTheDocument();
await select(selectEl, 'bar', { container: document.body });
expect(input.fetch).toHaveBeenCalled();
expect(onOptionsChange).toHaveBeenCalledWith({
...props.options,
jsonData: {
...props.options.jsonData,
foo: 'bar',
},
});
});

it('should call custom onChange', async () => {
const onChange = jest.fn();
const input: SelectorInput = {
id: 'foo',
label: 'foo-id',
fetch: jest.fn().mockResolvedValue(['bar']),
onChange,
};
render(<Selector {...props} input={input} />);
const selectEl = screen.getByLabelText(input.label);
expect(selectEl).toBeInTheDocument();
await select(selectEl, 'bar', { container: document.body });
expect(input.fetch).toHaveBeenCalled();
expect(onChange).toHaveBeenCalled();
});
});
71 changes: 71 additions & 0 deletions src/sql/ConfigEditor/Selector.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import React from 'react';
import { DataSourcePluginOptionsEditorProps, SelectableValue } from '@grafana/data';
import { AwsAuthDataSourceJsonData, AwsAuthDataSourceSecureJsonData } from '../../types';
import { ResourceSelector } from '../ResourceSelector';

export type SelectorInput = {
id: string;
fetch: () => Promise<Array<string | SelectableValue<string>>>;
dependencies?: string[];
label?: string;
'data-testid'?: string;
hidden?: boolean;
disabled?: boolean;
onChange?: (e: SelectableValue<string>) => void;
value?: string;
};

type SQLDataSourceJsonData<T extends AwsAuthDataSourceJsonData> = T & {
[n: string]: any;
};

export interface SelectorProps
extends DataSourcePluginOptionsEditorProps<SQLDataSourceJsonData<{}>, AwsAuthDataSourceSecureJsonData> {
input: SelectorInput;
saveOptions: () => Promise<void>;
}

export function Selector(props: SelectorProps) {
const { jsonData } = props.options;
const commonProps = {
title: jsonData.defaultRegion ? '' : 'select a default region',
disabled: !jsonData.defaultRegion,
labelWidth: 28,
className: 'width-30',
};

// The input is a selector
const { input } = props;
const onChange = (e: SelectableValue<string> | null) => {
if (input.onChange) {
input.onChange(e);
} else {
props.onOptionsChange({
...props.options,
jsonData: {
...props.options.jsonData,
[input.id]: e ? e.value || '' : e,
},
});
}
};
const dependencies: string[] = [];
if (input.dependencies) {
input.dependencies.forEach((dep) => dependencies.push(props.options.jsonData[dep]));
}
return (
<ResourceSelector
key={input.id}
label={input.label}
data-testid={input['data-testid']}
onChange={onChange}
fetch={input.fetch}
value={input.value || props.options.jsonData[input.id]}
saveOptions={props.saveOptions}
dependencies={dependencies}
hidden={input.hidden}
disabled={input.disabled}
{...commonProps}
/>
);
}
Loading