feat: Picker - formatter settings (#1907)

- useFormatter hook + optional settings prop in jsapi Picker
- Fixed a bug with `bindAllMethods` function. It now excludes getters

resolves #1889

Author: bmingles

packages/jsapi-components/src/index.ts

@@ -11,6 +11,7 @@ export { default as useDebouncedViewportSearch } from './useDebouncedViewportSea
 export * from './useDebouncedViewportSelectionFilter';
 export * from './useFilterConditionFactories';
 export * from './useFilteredItemsWithDefaultValue';
+export * from './useFormatter';
 export * from './useGetItemIndexByValue';
 export * from './useGetItemPosition';
 export { default as useInitializeViewportData } from './useInitializeViewportData';

packages/jsapi-components/src/spectrum/Picker/Picker.tsx

@@ -3,12 +3,12 @@ import {
   Picker as PickerBase,
   PickerProps as PickerPropsBase,
 } from '@deephaven/components';
-import { useApi } from '@deephaven/jsapi-bootstrap';
 import { dh as DhType } from '@deephaven/jsapi-types';
-import { Formatter } from '@deephaven/jsapi-utils';
+import { Settings } from '@deephaven/jsapi-utils';
 import Log from '@deephaven/log';
 import { PICKER_ITEM_HEIGHT, PICKER_TOP_OFFSET } from '@deephaven/utils';
 import { useCallback, useEffect, useMemo } from 'react';
+import useFormatter from '../../useFormatter';
 import useGetItemIndexByValue from '../../useGetItemIndexByValue';
 import { useViewportData } from '../../useViewportData';
 import { getPickerKeyColumn } from './PickerUtils';
@@ -24,21 +24,19 @@ export interface PickerProps extends Omit<PickerPropsBase, 'children'> {
   labelColumn?: string;
 
   // TODO #1890 : descriptionColumn, iconColumn
+
+  settings?: Settings;
 }
 
 export function Picker({
   table,
   keyColumn: keyColumnName,
   labelColumn: labelColumnName,
   selectedKey,
+  settings,
   ...props
 }: PickerProps): JSX.Element {
-  const dh = useApi();
-
-  const formatValue = useMemo(() => {
-    const formatter = new Formatter(dh);
-    return formatter.getFormattedString.bind(formatter);
-  }, [dh]);
+  const { getFormattedString: formatValue } = useFormatter(settings);
 
   const keyColumn = useMemo(
     () => getPickerKeyColumn(table, keyColumnName),
@@ -100,7 +98,7 @@ export function Picker({
         isCanceled = true;
       };
     },
-    [getItemIndexByValue, setViewport]
+    [getItemIndexByValue, settings, setViewport]
   );
 
   return (

packages/jsapi-components/src/useFormatter.test.ts

@@ -0,0 +1,69 @@
+import { useApi } from '@deephaven/jsapi-bootstrap';
+import { bindAllMethods, TestUtils } from '@deephaven/utils';
+import {
+  createFormatterFromSettings,
+  Formatter,
+  Settings,
+} from '@deephaven/jsapi-utils';
+import type { dh as DhType } from '@deephaven/jsapi-types';
+import { renderHook } from '@testing-library/react-hooks';
+import { useFormatter } from './useFormatter';
+
+jest.mock('@deephaven/jsapi-bootstrap');
+jest.mock('@deephaven/jsapi-utils', () => {
+  const actual = jest.requireActual('@deephaven/jsapi-utils');
+  return {
+    ...actual,
+    createFormatterFromSettings: jest.fn(),
+  };
+});
+jest.mock('@deephaven/utils', () => ({
+  ...jest.requireActual('@deephaven/utils'),
+  bindAllMethods: jest.fn(),
+}));
+
+const { asMock, createMockProxy } = TestUtils;
+
+beforeEach(() => {
+  jest.clearAllMocks();
+  expect.hasAssertions();
+});
+
+describe('useFormatter', () => {
+  const mock = {
+    dh: createMockProxy<typeof DhType>(),
+    formatter: createMockProxy<Formatter>(),
+    settings: createMockProxy<Settings>(),
+  };
+
+  beforeEach(() => {
+    asMock(useApi).mockReturnValue(mock.dh);
+
+    asMock(bindAllMethods)
+      .mockName('bindAllMethods')
+      .mockImplementation(a => a);
+
+    asMock(createFormatterFromSettings)
+      .mockName('createFormatterFromSettings')
+      .mockReturnValue(mock.formatter);
+  });
+
+  it('should return members of a `Formatter` instance based on settings', () => {
+    const { result } = renderHook(() => useFormatter(mock.settings));
+
+    expect(createFormatterFromSettings).toHaveBeenCalledWith(
+      mock.dh,
+      mock.settings
+    );
+
+    expect(bindAllMethods).toHaveBeenCalledWith(mock.formatter);
+
+    expect(result.current).toEqual({
+      getColumnFormat: mock.formatter.getColumnFormat,
+      getColumnFormatMapForType: mock.formatter.getColumnFormatMapForType,
+      getColumnTypeFormatter: mock.formatter.getColumnTypeFormatter,
+      getFormattedString: mock.formatter.getFormattedString,
+      timeZone: mock.formatter.timeZone,
+    });
+  });
+});

packages/jsapi-components/src/useFormatter.ts

@@ -0,0 +1,55 @@
+import { useApi } from '@deephaven/jsapi-bootstrap';
+import {
+  createFormatterFromSettings,
+  Formatter,
+  Settings,
+} from '@deephaven/jsapi-utils';
+import { bindAllMethods } from '@deephaven/utils';
+import { useMemo } from 'react';
+
+export type UseFormatterResult = Pick<
+  Formatter,
+  | 'getColumnFormat'
+  | 'getColumnFormatMapForType'
+  | 'getColumnTypeFormatter'
+  | 'getFormattedString'
+  | 'timeZone'
+>;
+
+/**
+ * Returns a subset of members of a `Formatter` instance. The `Formatter` will be
+ * constructed based on the given options or fallback to the configuration found
+ * in the current `FormatSettingsContext`. Members that are functions are bound
+ * to the `Formatter` instance, so they are safe to destructure. Static methods
+ * can still be accessed statically from the `Formatter` class.
+ * @param settings Optional settings to use when constructing the `Formatter`
+ */
+export function useFormatter(settings?: Settings): UseFormatterResult {
+  const dh = useApi();
+
+  const formatter = useMemo(() => {
+    const instance = createFormatterFromSettings(dh, settings);
+
+    // Bind all methods so we can destructure them
+    bindAllMethods(instance);
+
+    return instance;
+  }, [dh, settings]);
+
+  const {
+    getColumnFormat,
+    getColumnFormatMapForType,
+    getColumnTypeFormatter,
+    getFormattedString,
+  } = formatter;
+
+  return {
+    getColumnFormat,
+    getColumnFormatMapForType,
+    getColumnTypeFormatter,
+    getFormattedString,
+    timeZone: formatter.timeZone,
+  };
+}
+
+export default useFormatter;

packages/jsapi-components/src/useGetItemIndexByValue.test.ts

@@ -54,6 +54,20 @@ describe('useGetItemIndexByValue', () => {
     }
   );
 
+  it('should throw if seekRow fails', async () => {
+    asMock(mockTable.seekRow).mockRejectedValue('Some error');
+
+    const { result } = renderHook(() =>
+      useGetItemIndexByValue({
+        columnName: 'mock.columnName',
+        table: mockTable,
+        value: 'mock.value',
+      })
+    );
+
+    expect(result.current()).rejects.toEqual('Some error');
+  });
+
   it.each([
     ['mock.value', 42, 42],
     ['mock.value', -1, null],

packages/jsapi-components/src/useGetItemIndexByValue.ts

@@ -31,7 +31,6 @@ export function useGetItemIndexByValue<TValue>({
     const columnValueType = tableUtils.getValueType(column.type);
 
     const index = await table.seekRow(0, column, columnValueType, value);
-
     return index === -1 ? null : index;
   }, [columnName, table, tableUtils, value]);
 }

packages/jsapi-utils/src/FormatterUtils.test.ts

@@ -1,5 +1,8 @@
-import Formatter from './Formatter';
+import dh from '@deephaven/jsapi-shim';
+import { TestUtils } from '@deephaven/utils';
+import Formatter, { FormattingRule } from './Formatter';
 import FormatterUtils, {
+  createFormatterFromSettings,
   getColumnFormats,
   getDateTimeFormatterOptions,
 } from './FormatterUtils';
@@ -9,7 +12,31 @@ import {
   TableColumnFormatType,
 } from './formatters';
 import TableUtils from './TableUtils';
-import { ColumnFormatSettings, DateTimeFormatSettings } from './Settings';
+import Settings, {
+  ColumnFormatSettings,
+  DateTimeFormatSettings,
+} from './Settings';
+
+jest.mock('./Formatter', () => {
+  const FormatterActual = jest.requireActual('./Formatter').default;
+
+  // Extract static methods
+  const { makeColumnFormatMap, makeColumnFormattingRule } = FormatterActual;
+
+  // Wrap Formatter constructor so we can spy on it
+  const mockFormatter = jest.fn((...args) => new FormatterActual(...args));
+
+  return {
+    __esModule: true,
+    default: Object.assign(mockFormatter, {
+      makeColumnFormatMap,
+      makeColumnFormattingRule,
+    }),
+  };
+});
+
+const { createMockProxy } = TestUtils;
+const FormatterActual = jest.requireActual('./Formatter').default;
 
 function makeFormatter(...settings: ConstructorParameters<typeof Formatter>) {
   return new Formatter(...settings);
@@ -23,6 +50,46 @@ function makeFormattingRule(
   return { label, formatString, type };
 }
 
+describe('createFormatterFromSettings', () => {
+  const mockSettings = createMockProxy<Settings>({
+    formatter: [
+      createMockProxy<FormattingRule>({
+        format: createMockProxy<TableColumnFormat>(),
+      }),
+    ],
+  });
+
+  it.each([undefined, mockSettings])(
+    'should create a formatter with the given settings: %s',
+    settings => {
+      const columnRules = getColumnFormats(settings);
+      const dateTimeOptions = getDateTimeFormatterOptions(settings);
+
+      const {
+        defaultDecimalFormatOptions,
+        defaultIntegerFormatOptions,
+        truncateNumbersWithPound,
+        showEmptyStrings,
+        showNullStrings,
+      } = settings ?? {};
+
+      const actual = createFormatterFromSettings(dh, settings);
+
+      expect(Formatter).toHaveBeenCalledWith(
+        dh,
+        columnRules,
+        dateTimeOptions,
+        defaultDecimalFormatOptions,
+        defaultIntegerFormatOptions,
+        truncateNumbersWithPound,
+        showEmptyStrings,
+        showNullStrings
+      );
+      expect(actual).toBeInstanceOf(FormatterActual);
+    }
+  );
+});
+
 describe('isCustomColumnFormatDefined', () => {
   const columnFormats = [
     Formatter.makeColumnFormattingRule(

packages/jsapi-utils/src/FormatterUtils.ts

@@ -1,7 +1,44 @@
+import type { dh as DhType } from '@deephaven/jsapi-types';
 import type { FormattingRule } from './Formatter';
 import Formatter from './Formatter';
 import { DateTimeColumnFormatter, TableColumnFormatter } from './formatters';
-import { ColumnFormatSettings, DateTimeFormatSettings } from './Settings';
+import Settings, {
+  ColumnFormatSettings,
+  DateTimeFormatSettings,
+} from './Settings';
+
+/**
+ * Instantiate a `Formatter` from the given settings.
+ * @param dh The `dh` object
+ * @param settings Optional settings to use
+ * @returns A new `Formatter` instance
+ */
+export function createFormatterFromSettings(
+  dh: typeof DhType,
+  settings?: Settings
+): Formatter {
+  const columnRules = getColumnFormats(settings);
+  const dateTimeOptions = getDateTimeFormatterOptions(settings);
+
+  const {
+    defaultDecimalFormatOptions,
+    defaultIntegerFormatOptions,
+    truncateNumbersWithPound,
+    showEmptyStrings,
+    showNullStrings,
+  } = settings ?? {};
+
+  return new Formatter(
+    dh,
+    columnRules,
+    dateTimeOptions,
+    defaultDecimalFormatOptions,
+    defaultIntegerFormatOptions,
+    truncateNumbersWithPound,
+    showEmptyStrings,
+    showNullStrings
+  );
+}
 
 export function getColumnFormats(
   settings?: ColumnFormatSettings
@@ -45,6 +82,7 @@ export function isCustomColumnFormatDefined(
 }
 
 export default {
+  createFormatterFromSettings,
   getColumnFormats,
   getDateTimeFormatterOptions,
   isCustomColumnFormatDefined,

packages/utils/src/ClassUtils.test.ts

@@ -7,6 +7,11 @@ class Aaa {
   getAaa() {
     return this.nameA;
   }
+
+  // eslint-disable-next-line class-methods-use-this
+  get someGetter() {
+    throw new Error('This should not get evaluated as a method');
+  }
 }
 
 class Bbb extends Aaa {
@@ -15,6 +20,11 @@ class Bbb extends Aaa {
   getBbb() {
     return this.nameB;
   }
+
+  // eslint-disable-next-line class-methods-use-this
+  get someGetter() {
+    throw new Error('This should not get evaluated as a method');
+  }
 }
 
 class Ccc extends Bbb {
@@ -25,6 +35,11 @@ class Ccc extends Bbb {
   }
 
   getCcc2 = () => this.nameC;
+
+  // eslint-disable-next-line class-methods-use-this
+  get someGetter() {
+    throw new Error('This should not get evaluated as a method');
+  }
 }
 
 beforeEach(() => {