Skip to content

Commit

Permalink
Merge branch 'main' into SavedlicenseA11yTests
Browse files Browse the repository at this point in the history
  • Loading branch information
bhavyarm authored Mar 10, 2022
2 parents 6283bc1 + 8cd75df commit 081ec2e
Show file tree
Hide file tree
Showing 57 changed files with 160 additions and 152 deletions.
99 changes: 93 additions & 6 deletions dev_docs/contributing/best_practices.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ tags: ['kibana', 'onboarding', 'dev', 'architecture']

## General

First things first, be sure to review our <DocLink id="kibDevPrinciples" text="development principles"/> and check out all the available
platform <DocLink id="kibBuildingBlocks" text="building blocks"/> that can simplify plugin development.

Be sure to follow our <DocLink id="kibDevPrinciples" text="development principles"/>
and <DocLink id="kibStandards" text="standards and guidelines"/>.
## Documentation

Documentation best practices can be found <DocLink id="kibDocumentation" text="here"/>.
Expand Down Expand Up @@ -54,11 +54,27 @@ We use es-lint rules when possible, but please review our [styleguide](https://g

Es-lint overrides on a per-plugin level are discouraged.

## Plugin best practices
## Using the SavedObjectClient

Don't export <DocLink id="kibPlatformIntro" section="public-plugin-api" text="public APIs"/> without reason. Make your public APIs as small as possible. You will have to maintain them, and consider backward compatibility when making changes.
The <DocLink id="kibCoreSavedObjectsPluginApi" section="def-public.SavedObjectsClient" text="SavedObjectClient" /> should always be used for reading and writing saved objects that you manage. For saved objects managed by other plugins, their plugin APIs should be used instead.

Good:
```
const dataView = dataViewStartContract.get(dataViewId);
```

Bad:
```
const dataView = savedObjectsClient.get(dataViewId) as DataView;
```

## Resusable react components

Add `README.md` to all your plugins and services and include contact information.
Use [EUI](https://elastic.github.io/eui) for all your basic UI components to create a consistent UI experience. We also have generic UI components offered from the <DocLink id="kibKibanaReactPluginApi" text="kibana_react" /> plugin and the <DocLink id="kibSharedUXPluginApi" text="shared_ux" /> plugin.

## Don't export code that doesn't need to be public

Don't export <DocLink id="kibPlatformIntro" section="public-plugin-api" text="public APIs"/> without reason. Make your public APIs as small as possible. You will have to maintain them, and consider backward compatibility when making changes.

## Re-inventing the wheel

Expand Down Expand Up @@ -120,6 +136,77 @@ There are some exceptions where a separate repo makes sense. However, they are e

It may be tempting to get caught up in the dream of writing the next package which is published to npm and downloaded millions of times a week. Knowing the quality of developers that are working on Kibana, this is a real possibility. However, knowing which packages will see mass adoption is impossible to predict. Instead of jumping directly to writing code in a separate repo and accepting all of the complications that come along with it, prefer keeping code inside the Kibana repo. A [Kibana package](https://github.com/elastic/kibana/tree/main/packages) can be used to publish a package to npm, while still keeping the code inside the Kibana repo. Move code to an external repo only when there is a good reason, for example to enable external contributions.

## Licensing

<DocCallOut title="Internal only">

Has there been a discussion about which license this feature should be available under? Open up a license issue in [https://github.com/elastic/dev](https://github.com/elastic/dev) if you are unsure.

</DocCallOut>

## Testing scenarios

Every PR submitted should be accompanied by tests. Read through the <DocLink id="kibDevTutorialTestingPlugins" text="testing plugins tutorial" /> for how to test.

### Browser coverage

Refer to the list of browsers and OS Kibana supports https://www.elastic.co/support/matrix

Does the feature work efficiently on the below listed browsers
- chrome
- Firefox
- Safari
- IE11

### Upgrade Scenarios
- Migration scenarios- Does the feature affect old indices, saved objects ?
- Has the feature been tested with Kibana aliases
- Read/Write privileges of the indices before and after the upgrade?

### Test coverage
- Does the feature have sufficient unit test coverage? (does it handle storeinSessions?)
- Does the feature have sufficient Functional UI test coverage?
- Does the feature have sufficient Rest API coverage test coverage?
- Does the feature have sufficient Integration test coverage?

### Environment configurations

- Kibana should be fully [cross cluster search](https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-cross-cluster-search.html) compatible (aside from admin UIs which only work on the local cluster).
- How does your plugin behave when optional dependencies are disabled?
- How does your app behave under anonymous access, or with security disabled?
- Make sure to test your PR in a cloud environment. Read about the <DocLink id="kibDevTutorialCI" section="labels" text="ci:deploy cloud" /> label which makes this very easy.


## Backward compatibility

Any time you change state that is part of a Saved Object you will have to write a <DocLink id="kibDevDocsSavedObjectsIntro" section="migrations-and-backward-compatibility" text="migration" />.

Never store state from another plugin in your Saved Objects or URLs unless it implements the <DocLink id="kibDevDocsPersistableStateIntro" text="persistable state interface"/>. Remember to check for migrations when deserializing that state.

If you expose state and you wish to allow other plugins to persist you must ensure it implements the <DocLink id="kibDevDocsPersistableStateIntro" text="persistable state interface"/>. This is very common for `by value` entities, like visualizations that exist on a dashboard but are not part of the visualization library. If you make a breaking change to this state you must remember to register a migration for it.

Saved objects exported from past Kibana versions should always continue to work. Bookmarked URLs should also always work. Check out <DocLink id="kibDevKeyConceptsNavigation" section="specifying-state" text="URL Locators" /> to learn about migrating state in URLs.

## Avoid these common mistakes

### Treating Kibana's filesystem as durable storage

Plugins should rarely, if ever, access Kibana's filesystem directly. Kibana instances are commonly ephemeral and anything written to the filesystem will potentially
not be there on restart.

### Storing state in server memory

There are generally multiple instances of Kibana all hosted behind a round-robin load-balancer. As a result, storing state in server memory is risky as there is no
guarantee that a single end-user's HTTP requests will be served by the same Kibana instance.

### Using WebSockets

Kibana has a number of platform services that don't work with WebSockets, for example authentication and authorization. If your use-case would benefit substantially
from websockets, talk to the Kibana Core team about adding support. Do not hack around this limitation, everytime that someone has, it's created so many problems
it's been eventually removed.



## Security best practices

When writing code for Kibana, be sure to follow these best practices to avoid common vulnerabilities. Refer to the included Open Web
Expand Down
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -719,9 +719,9 @@
"@types/yargs": "^15.0.0",
"@types/yauzl": "^2.9.1",
"@types/zen-observable": "^0.8.0",
"@typescript-eslint/eslint-plugin": "^5.13.0",
"@typescript-eslint/parser": "^5.13.0",
"@typescript-eslint/typescript-estree": "^5.13.0",
"@typescript-eslint/eslint-plugin": "^5.14.0",
"@typescript-eslint/parser": "^5.14.0",
"@typescript-eslint/typescript-estree": "^5.14.0",
"@yarnpkg/lockfile": "^1.1.0",
"abab": "^2.0.4",
"aggregate-error": "^3.1.0",
Expand Down
16 changes: 16 additions & 0 deletions packages/elastic-eslint-config-kibana/typescript.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,22 @@ module.exports = {
selector: 'enum',
format: ['PascalCase', 'UPPER_CASE', 'camelCase'],
},
// https://typescript-eslint.io/rules/naming-convention/#ignore-properties-that-require-quotes
// restore check behavior before https://github.com/typescript-eslint/typescript-eslint/pull/4582
{
selector: [
'classProperty',
'objectLiteralProperty',
'typeProperty',
'classMethod',
'objectLiteralMethod',
'typeMethod',
'accessor',
'enumMember'
],
format: null,
modifiers: ['requiresQuotes']
}
],
'@typescript-eslint/explicit-member-accessibility': ['error',
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ describe('checkCompatibleTypeDescriptor', () => {
]);
expect(incompatibles).toHaveLength(1);
const { diff, message } = incompatibles[0];
// eslint-disable-next-line @typescript-eslint/naming-convention
expect(diff).toEqual({ '@@INDEX@@.count_2.kind': 'number' });
expect(message).toHaveLength(1);
expect(message).toEqual([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -616,9 +616,7 @@ export class Field extends PureComponent<FieldProps> {
const isInvalid = unsavedChanges?.isInvalid;

const className = classNames('mgtAdvancedSettings__field', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'mgtAdvancedSettings__field--unsaved': unsavedChanges,
// eslint-disable-next-line @typescript-eslint/naming-convention
'mgtAdvancedSettings__field--invalid': isInvalid,
});
const groupId = `${setting.name}-group`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,8 @@ export const MetricVisValue = ({
autoScale,
}: MetricVisValueProps) => {
const containerClassName = classNames('mtrVis__container', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'mtrVis__container--light': metric.lightText,
// eslint-disable-next-line @typescript-eslint/naming-convention
'mtrVis__container-isfilterable': onFilter,
// eslint-disable-next-line @typescript-eslint/naming-convention
'mtrVis__container-isfull': !autoScale && colorFullBackground,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ export const ColorPicker = ({
size="l"
color={selectedColor}
className={classNames('visColorPicker__valueDot', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'visColorPicker__valueDot-isSelected': color === selectedColor,
})}
style={{ color }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ const LegendToggleComponent = ({ onClick, showLegend, legendPosition }: LegendTo
color="text"
onClick={onClick}
className={classNames('echLegend__toggle', `echLegend__toggle--position-${legendPosition}`, {
// eslint-disable-next-line @typescript-eslint/naming-convention
'echLegend__toggle--isOpen': showLegend,
})}
aria-label={i18n.translate('charts.legend.toggleLegendButtonAriaLabel', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ const Item = React.forwardRef<HTMLDivElement, Props>(
const expandPanel = expandedPanelId !== undefined && expandedPanelId === id;
const hidePanel = expandedPanelId !== undefined && expandedPanelId !== id;
const classes = classNames({
// eslint-disable-next-line @typescript-eslint/naming-convention
'dshDashboardGrid__item--expanded': expandPanel,
// eslint-disable-next-line @typescript-eslint/naming-convention
'dshDashboardGrid__item--hidden': hidePanel,
// eslint-disable-next-line @typescript-eslint/naming-convention
printViewport__vis: container.getInput().viewMode === ViewMode.PRINT,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ export const QueryBarTopRow = React.memo(
}

const wrapperClasses = classNames('kbnQueryBar__datePickerWrapper', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'kbnQueryBar__datePickerWrapper-isHidden': isQueryInputFocused,
});

Expand Down
1 change: 0 additions & 1 deletion src/plugins/data/public/ui/search_bar/search_bar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,6 @@ class SearchBarUI extends Component<SearchBarProps, State> {
let filterBar;
if (this.shouldRenderFilterBar()) {
const filterGroupClasses = classNames('globalFilterGroup__wrapper', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'globalFilterGroup__wrapper-isVisible': this.state.isFiltersVisible,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,13 @@ export const Panel: React.FC<Props & React.HTMLProps<HTMLDivElement>> = ({
hasFooter: false,
});

/* eslint-disable @typescript-eslint/naming-convention */
const classes = classnames('fieldEditor__flyoutPanel', className, {
'fieldEditor__flyoutPanel--pageBackground': backgroundColor === 'euiPageBackground',
'fieldEditor__flyoutPanel--emptyShade': backgroundColor === 'euiEmptyShade',
'fieldEditor__flyoutPanel--leftBorder': border === 'left',
'fieldEditor__flyoutPanel--rightBorder': border === 'right',
'fieldEditor__flyoutPanel--withContent': config.hasContent,
});
/* eslint-enable @typescript-eslint/naming-convention */

const { addPanel } = useFlyoutPanelsContext();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,13 @@ export const Panel: React.FC<Props & React.HTMLProps<HTMLDivElement>> = ({

const [styles, setStyles] = useState<CSSProperties>({});

/* eslint-disable @typescript-eslint/naming-convention */
const classes = classnames('fieldEditor__flyoutPanel', className, {
'fieldEditor__flyoutPanel--pageBackground': backgroundColor === 'euiPageBackground',
'fieldEditor__flyoutPanel--emptyShade': backgroundColor === 'euiEmptyShade',
'fieldEditor__flyoutPanel--leftBorder': border === 'left',
'fieldEditor__flyoutPanel--rightBorder': border === 'right',
'fieldEditor__flyoutPanel--withContent': config.hasContent,
});
/* eslint-enable @typescript-eslint/naming-convention */

const { addPanel } = useFlyoutPanelsContext();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,10 @@ export const PreviewListItem: React.FC<Props> = ({

const [isPreviewImageModalVisible, setIsPreviewImageModalVisible] = useState(false);

/* eslint-disable @typescript-eslint/naming-convention */
const classes = classnames('indexPatternFieldEditor__previewFieldList__item', {
'indexPatternFieldEditor__previewFieldList__item--highlighted': isFromScript,
'indexPatternFieldEditor__previewFieldList__item--pinned': isPinned,
});
/* eslint-enable @typescript-eslint/naming-convention */

const doesContainImage = formattedValue?.includes('<img');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,6 @@ export function DiscoverGridDocumentToolbarBtn({
className={classNames({
// eslint-disable-next-line @typescript-eslint/naming-convention
euiDataGrid__controlBtn: true,
// eslint-disable-next-line @typescript-eslint/naming-convention
'euiDataGrid__controlBtn--active': isFilterActive,
})}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ export const TableRow = ({
);
const [open, setOpen] = useState(false);
const docTableRowClassName = classNames('kbnDocTable__row', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'kbnDocTable__row--highlight': row.isAnchor,
});
const anchorDocTableRowSubj = row.isAnchor ? ' docTableAnchorRow' : '';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ export function PanelHeader({
const showPanelBar =
!isViewMode || badges.length > 0 || notifications.length > 0 || showTitle || description;
const classes = classNames('embPanel__header', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'embPanel__header--floater': !showPanelBar,
});
const placeholderTitle = i18n.translate('embeddableApi.panel.placeholderTitle', {
Expand Down
2 changes: 0 additions & 2 deletions src/plugins/kibana_react/public/code_editor/code_editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,9 @@ export const CodeEditor: React.FC<Props> = ({

const [isHintActive, setIsHintActive] = useState(true);

/* eslint-disable @typescript-eslint/naming-convention */
const promptClasses = classNames('kibanaCodeEditor__keyboardHint', {
'kibanaCodeEditor__keyboardHint--isInactive': !isHintActive,
});
/* eslint-enable @typescript-eslint/naming-convention */

const _updateDimensions = useCallback(() => {
_editor.current?.layout();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ export const withSolutionNav = (WrappedComponent: ComponentType<KibanaPageTempla
const sideBarClasses = classNames(
'kbnPageTemplate__pageSideBar',
{
// eslint-disable-next-line @typescript-eslint/naming-convention
'kbnPageTemplate__pageSideBar--shrink':
isMediumBreakpoint || (isLargerBreakpoint && !isSideNavOpenOnDesktop),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,6 @@ export const stackManagementSchema: MakeSchemaFrom<UsageStats> = {
type: 'boolean',
_meta: { description: 'Non-default value of setting.' },
},
// eslint-disable-next-line @typescript-eslint/naming-convention
'doc_table:hideTimeColumn': {
type: 'boolean',
_meta: { description: 'Non-default value of setting.' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ export interface UsageStats {
'notifications:lifetime:error': number;
'doc_table:highlight': boolean;
'discover:searchOnPageLoad': boolean;
// eslint-disable-next-line @typescript-eslint/naming-convention
'doc_table:hideTimeColumn': boolean;
'discover:sampleSize': number;
defaultColumns: string[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,6 @@ export class VisLegend extends PureComponent<VisLegendProps, VisLegendState> {
type="button"
onClick={this.toggleLegend}
className={classNames('visLegend__toggle kbn-resetFocusState', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'visLegend__toggle--isOpen': open,
})}
aria-label={i18n.translate('visTypeVislib.vislib.legend.toggleLegendButtonAriaLabel', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ export const ColumnChart: FC<Props> = ({
)}
<div
className={classNames('dataGridChart__legend', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'dataGridChart__legend--numeric': columnType.schema === 'number',
})}
data-test-subj={`${dataTestSubj}-legend`}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ export function FieldEditor({
color={initialField.color}
iconSide="right"
className={classNames('gphFieldEditor__badge', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'gphFieldEditor__badge--disabled': isDisabled,
})}
onClickAriaLabel={badgeDescription}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ export function FieldPicker({
<EuiBadge
data-test-subj="graph-add-field-button"
className={classNames('gphFieldPicker__button', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'gphFieldPicker__button--disabled': !hasFields,
})}
color="hollow"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,15 +147,13 @@ export function GraphVisualization({
cy={node.ky}
r={node.scaledSize}
className={classNames('gphNode__circle', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'gphNode__circle--selected': node.isSelected,
})}
style={{ fill: node.color }}
/>
{node.icon && (
<text
className={classNames('fa gphNode__text', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'gphNode__text--inverse': isColorDark(...hexToRgb(node.color)),
})}
transform="translate(0,5)"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ function ListItem({
// eslint-disable-next-line jsx-a11y/role-supports-aria-props
<li
className={classNames('gphGuidancePanel__item', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'gphGuidancePanel__item--disabled': state === 'disabled',
})}
aria-disabled={state === 'disabled'}
Expand All @@ -62,7 +61,6 @@ function ListItem({
{state !== 'disabled' && (
<span
className={classNames('gphGuidancePanel__itemIcon', {
// eslint-disable-next-line @typescript-eslint/naming-convention
'gphGuidancePanel__itemIcon--done': state === 'done',
})}
aria-hidden={true}
Expand Down
Loading

0 comments on commit 081ec2e

Please sign in to comment.