Skip to content

Commit

Permalink
[Field Formatters] Fix field format convert() return value (#211342)
Browse files Browse the repository at this point in the history
## Summary

It was [pointed
out](#209824 (comment))
during a recent review that the field format `convert()` value does some
odd casting. It looks like this probably isn't necessary since
`converter` should never be undefined, and we can just return the result
directly.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Elastic Machine <[email protected]>
  • Loading branch information
davismcphee and elasticmachine authored Feb 26, 2025
1 parent 76ae900 commit 443fd5b
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { sharePluginMock } from '@kbn/share-plugin/public/mocks';
import { coreMock as corePluginMock } from '@kbn/core/public/mocks';
import { DataTableRecord, buildDataTableRecord } from '@kbn/discover-utils';
import { dataViewMock } from '@kbn/discover-utils/src/__mocks__/data_view';
import type { IFieldFormatsRegistry } from '@kbn/field-formats-plugin/common';

jest.mock('@elastic/eui', () => ({
...jest.requireActual('@elastic/eui'),
Expand All @@ -46,7 +47,15 @@ const getSummaryProps = (
isDetails: false,
row: record,
dataView: dataViewMock,
fieldFormats: fieldFormatsMock,
fieldFormats: {
...fieldFormatsMock,
getDefaultInstance: jest
.fn()
.mockImplementation((...params: Parameters<IFieldFormatsRegistry['getDefaultInstance']>) => ({
...fieldFormatsMock.getDefaultInstance(...params),
convert: jest.fn().mockImplementation((t: string) => String(t)),
})),
},
setCellProps: () => {},
closePopover: () => {},
density: DataGridDensity.COMPACT,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,7 @@ export const createResourceFields = ({
return {
name,
rawValue: resourceDoc[name],
// TODO: formatFieldValue doesn't actually return a string in certain circumstances, change
// this line below once it does.
value: typeof value === 'string' ? value : `${value}`,
value,
ResourceBadge: getResourceBadgeComponent(name, core, share),
Icon: getResourceBadgeIcon(name, resourceDoc),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,7 @@ export const getServiceNameCell =
onFilter={actions.addFilter}
icon={getIcon}
rawValue={serviceNameValue}
// TODO: formatFieldValue doesn't actually return a string in certain circumstances, change
// this line below once it does.
value={typeof value === 'string' ? value : `${value}`}
value={value}
property={serviceNameField}
core={core}
share={share}
Expand Down
11 changes: 2 additions & 9 deletions src/platform/plugins/shared/field_formats/common/field_format.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,7 @@ export abstract class FieldFormat {
contentType: FieldFormatsContentType = DEFAULT_CONTEXT_TYPE,
options?: HtmlContextTypeOptions | TextContextTypeOptions
): string {
const converter = this.getConverterFor(contentType);

if (converter) {
return converter.call(this, value, options);
}

// TODO: should be "return `${value}`;", but might be a breaking change
return value as string;
return this.getConverterFor(contentType).call(this, value, options);
}

/**
Expand All @@ -140,7 +133,7 @@ export abstract class FieldFormat {
this.convertObject = this.setupContentType();
}

return this.convertObject[contentType];
return this.convertObject[contentType] ?? this.convertObject.text;
}

/**
Expand Down

0 comments on commit 443fd5b

Please sign in to comment.