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

feat(react-utilities): make dictionaries strict and get rid of unused generics to improve DX and type safety #23019

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
88df0e6
feat(react-utilities): make dicionaries strict and get rid of unused …
Hotell May 16, 2022
e32a270
chore: add eslint-plugin-etc and enable no-missused-generics rule to …
Hotell May 16, 2022
223cc42
fixup! feat(react-utilities): make dicionaries strict and get rid of …
Hotell May 16, 2022
1967330
docs(react-utilities): re-generate api
Hotell May 16, 2022
dc36c46
chore(react-utilities): fix build:local task via migration-generator
Hotell May 16, 2022
5b3d376
generate change file
Hotell May 16, 2022
f88fed4
fixup! chore: add eslint-plugin-etc and enable no-missused-generics r…
Hotell May 16, 2022
2c4e6fc
fixup! fixup! feat(react-utilities): make dicionaries strict and get …
Hotell May 19, 2022
6458f80
fix: explicitly assert components getNativeElemProps calls that are i…
Hotell May 19, 2022
1f7742b
fixup! docs(react-utilities): re-generate api
Hotell May 20, 2022
3fe93d4
fixup! fixup! fixup! feat(react-utilities): make dicionaries strict a…
Hotell May 20, 2022
2ad53fc
fixup! fixup! fixup! fixup! feat(react-utilities): make dicionaries s…
Hotell May 20, 2022
4af9e8c
fixup! fixup! docs(react-utilities): re-generate api
Hotell May 20, 2022
2439478
generate change files for updated packages
Hotell May 20, 2022
4fdb516
test(react-utils): add more tests
Hotell May 20, 2022
ee3513e
feat: final fantasy ?
Hotell May 20, 2022
359e8fa
docs: re-generate api
Hotell May 20, 2022
2411870
fixup! feat: final fantasy ?
Hotell May 20, 2022
10aae1f
fixup! fixup! feat: final fantasy ?
Hotell May 20, 2022
c644123
fixup! fixup! fixup! feat: final fantasy ?
Hotell May 20, 2022
5aa2409
fixup! fixup! fixup! fixup! feat: final fantasy ?
Hotell May 20, 2022
c82afa3
fixup! docs: re-generate api
Hotell May 20, 2022
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "fix: explicitly assert components getNativeElemProps calls that are incompatible with Slot api",
"packageName": "@fluentui/react-button",
"email": "[email protected]",
"dependentChangeType": "none"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "fix: explicitly assert components getNativeElemProps calls that are incompatible with Slot api",
"packageName": "@fluentui/react-link",
"email": "[email protected]",
"dependentChangeType": "none"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "fix: explicitly assert components getNativeElemProps calls that are incompatible with Slot api",
"packageName": "@fluentui/react-menu",
"email": "[email protected]",
"dependentChangeType": "none"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "fix: explicitly assert components getNativeElemProps calls that are incompatible with Slot api",
"packageName": "@fluentui/react-text",
"email": "[email protected]",
"dependentChangeType": "none"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "feat: make dictionaries and getNativeElementProps strict and get rid of unused generics to improve DX and type safety",
"packageName": "@fluentui/react-utilities",
"email": "[email protected]",
"dependentChangeType": "patch"
}
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"check:change": "beachball check --scope \"!packages/fluentui/*\"",
"check:modified-files": "yarn workspace @fluentui/scripts just check-for-modified-files",
"check:affected-package": "node ./scripts/monorepo/checkIfPackagesAffected.js",
"check:installed-dependencies-versions": "satisfied --skip-invalid --ignore \"prettier|angular|lit|sass|@storybook/web-components|@storybook/html|svelte|@testing-library|vue|@cypress/react\"",
"check:installed-dependencies-versions": "satisfied --skip-invalid --ignore \"prettier|angular|lit|sass|@storybook/web-components|@storybook/html|svelte|@testing-library|vue|@cypress/react|eslint-plugin-etc\"",
"clean": "lage clean --verbose",
"code-style": "lage code-style --verbose",
"codepen": "cd packages/react && node ../../scripts/local-codepen.js",
Expand Down Expand Up @@ -181,6 +181,7 @@
"eslint-import-resolver-typescript": "2.5.0",
"eslint-plugin-deprecation": "1.2.1",
"eslint-plugin-es": "4.1.0",
"eslint-plugin-etc": "2.0.2",
"eslint-plugin-import": "2.25.4",
"eslint-plugin-jest": "23.20.0",
"eslint-plugin-jsdoc": "^36.0.7",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ export const useButton_unstable = (
root: 'button',
icon: 'span',
},

root: getNativeElementProps(
as || 'button',
useARIAButton<ARIAButtonSlotProps>(props, {
Expand All @@ -56,7 +55,7 @@ export const useButton_unstable = (
type: 'button', // This is added because the default for type is 'submit'
},
}),
),
) as ButtonState['root'],
Copy link
Contributor Author

@Hotell Hotell May 20, 2022

Choose a reason for hiding this comment

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

Because we now have proper getNativeProps types, elements that can have variadic root cannot match with original Props definition where Slot mapped type is used to define the api.

Why?

Slot api design is cannot match with implementation/runtime - Slot uses binary arguments, where 1st generic can only be a 1st string. from component API perspective that's impossible to achieve as Slot is always 1 string or union of multiple strings -> we cannot extract first type from union nor transform unions to arrays.

// impossible transform in TypeScript ❌

type Element = 'div' | 'span' | 'section' 

↓↓↓

type SlotCompatibleType = <'div' , 'span' | 'section'> 

icon: iconShorthand,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const useLink_unstable = (
ref,
...props,
as,
}),
}) as LinkState['root'],
};

useLinkState_unstable(state);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,15 @@ export const useMenuPopover_unstable = (props: MenuPopoverProps, ref: React.Ref<

const { onMouseEnter: onMouseEnterOriginal, onKeyDown: onKeyDownOriginal } = rootProps;

rootProps.onMouseEnter = useEventCallback((e: React.MouseEvent<HTMLElement>) => {
rootProps.onMouseEnter = useEventCallback((e: React.MouseEvent<HTMLDivElement>) => {
Copy link
Contributor Author

@Hotell Hotell May 20, 2022

Choose a reason for hiding this comment

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

because we now have strict type checking and inference, we need to provide correct types instead of wide types

if (openOnHover) {
setOpen(e, { open: true, keyboard: false });
}

onMouseEnterOriginal?.(e);
});

rootProps.onKeyDown = useEventCallback((e: React.KeyboardEvent<HTMLElement>) => {
rootProps.onKeyDown = useEventCallback((e: React.KeyboardEvent<HTMLDivElement>) => {
const key = e.key;

if (key === Escape || (isSubmenu && key === CloseArrowKey)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const useText_unstable = (props: TextProps, ref: React.Ref<HTMLElement>):
ref,
...props,
as,
}),
}) as TextState['root'],
};

return state;
Expand Down
6 changes: 5 additions & 1 deletion packages/react-components/react-utilities/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
{
"extends": ["plugin:@fluentui/eslint-plugin/react"],
"root": true
"plugins": ["etc"],
"root": true,
"rules": {
"etc/no-misused-generics": "warn"
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"$schema": "https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json",
"extends": "./api-extractor.json",
"mainEntryPointFilePath": "<projectFolder>/dist/packages/react-components/<unscopedPackageName>/src/index.d.ts"
"mainEntryPointFilePath": "<projectFolder>/dist/types/packages/react-components/<unscopedPackageName>/src/index.d.ts"
}

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion packages/react-components/react-utilities/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"lint": "just-scripts lint",
"test": "jest --passWithNoTests",
"docs": "api-extractor run --config=config/api-extractor.local.json --local",
"build:local": "tsc -p ./tsconfig.lib.json --module esnext --emitDeclarationOnly && node ../../../scripts/typescript/normalize-import --output ./dist/packages/react-components/react-utilities/src && yarn docs",
"build:local": "tsc -p ./tsconfig.lib.json --module esnext --emitDeclarationOnly && node ../../../scripts/typescript/normalize-import --output ./dist/types/packages/react-components/react-utilities/src && yarn docs",
"type-check": "tsc -b tsconfig.json"
},
"devDependencies": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ type EmptyIntrisicElements =
* * Removes legacy string ref.
* * Disallows children for empty tags like 'img'.
*/
type IntrisicElementProps<Type extends keyof JSX.IntrinsicElements> = React.PropsWithRef<JSX.IntrinsicElements[Type]> &
export type IntrisicElementProps<Type extends keyof JSX.IntrinsicElements> = React.PropsWithRef<
JSX.IntrinsicElements[Type]
> &
(Type extends EmptyIntrisicElements ? { children?: never } : {});

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import { getNativeElementProps } from './getNativeElementProps';

describe('getNativeElementProps', () => {
it('can filter native element properties', () => {
expect(getNativeElementProps('div', { id: '123', checked: true })).toEqual({ id: '123' });
expect(getNativeElementProps('div', { id: '123', checked: true })).toEqual({
id: '123',
});
expect(getNativeElementProps('input', { id: '123', checked: true })).toEqual({ id: '123', checked: true });
expect(getNativeElementProps('input', { id: '123', checked: true }, ['id'])).toEqual({ checked: true });
});
Expand All @@ -12,6 +14,10 @@ describe('getNativeElementProps', () => {
});

it('excludes props regardless of the allowed', () => {
expect(getNativeElementProps('div', { as: 'span' }, ['as'])).toEqual({});
const actual = getNativeElementProps('div', { as: 'span' }, ['as']);

// @ts-expect-error -- `as` was removed from the object
expect(actual.as).toBeUndefined();
expect(actual).toEqual({});
});
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import * as React from 'react';
import type { IntrisicElementProps } from '../compose';

import {
labelProperties,
audioProperties,
Expand Down Expand Up @@ -26,7 +28,7 @@ import {
timeProperties,
} from './properties';

const nativeElementMap: Record<string, Record<string, number>> = {
const nativeElementMap = {
label: labelProperties,
audio: audioProperties,
video: videoProperties,
Expand All @@ -50,6 +52,14 @@ const nativeElementMap: Record<string, Record<string, number>> = {
img: imgProperties,
time: timeProperties,
};
type NativeElementMap = typeof nativeElementMap;

/**
* AsTagNames - can single string or union of strings
*/
type NativeElemProps<AsTagNames extends keyof JSX.IntrinsicElements> = {
[As in AsTagNames]: { as?: As } & IntrisicElementProps<As>;
}[AsTagNames];

/**
* Given an element tagname and user props, filters the props to only allowed props for the given
Expand All @@ -58,16 +68,21 @@ const nativeElementMap: Record<string, Record<string, number>> = {
* @param props - Props object
* @param excludedPropNames - List of props to disallow
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function getNativeElementProps<TAttributes extends React.HTMLAttributes<any>>(
tagName: string,
props: {},
excludedPropNames?: string[],
): TAttributes {
const allowedPropNames = (tagName && nativeElementMap[tagName]) || htmlElementProperties;
allowedPropNames.as = 1;
export function getNativeElementProps<
Tag extends keyof JSX.IntrinsicElements,
Props extends Record<string, unknown>,
ExcludedPropKeys extends Extract<keyof Props, string> = never
>(tagName: Tag, props: Props, excludedPropNames?: ExcludedPropKeys[]): Omit<NativeElemProps<Tag>, ExcludedPropKeys> {
const allowedPropNames = nativeElementMap[tagName as keyof NativeElementMap] || htmlElementProperties;

/**
* extends object dictionary with `as` to avoid adding it to nativeElementMap and htmlElementProperties
*/
type ExtendedAllowedPropNames = typeof allowedPropNames & { as: 1 };

(allowedPropNames as ExtendedAllowedPropNames).as = 1;

return getNativeProps(props, allowedPropNames, excludedPropNames);
return getNativeProps(props, allowedPropNames, excludedPropNames) as Omit<NativeElemProps<Tag>, ExcludedPropKeys>;
}

/**
Expand All @@ -79,15 +94,16 @@ export function getNativeElementProps<TAttributes extends React.HTMLAttributes<a
* @returns An object containing the native props for the `root` and primary slots.
*/
export const getPartitionedNativeProps = <
Props extends Pick<React.HTMLAttributes<HTMLElement>, 'style' | 'className'>,
Tag extends keyof JSX.IntrinsicElements,
Props extends Record<string, unknown> & Pick<React.HTMLAttributes<HTMLElement>, 'style' | 'className'>,
ExcludedPropKeys extends Extract<keyof Props, string> = never
>({
primarySlotTagName,
props,
excludedPropNames,
}: {
/** The primary slot's element type (e.g. 'div') */
primarySlotTagName: keyof JSX.IntrinsicElements;
primarySlotTagName: Tag;

/** The component's props object */
props: Props;
Expand All @@ -97,10 +113,10 @@ export const getPartitionedNativeProps = <
}) => {
return {
root: { style: props.style, className: props.className },
primary: getNativeElementProps<Omit<Props, ExcludedPropKeys>>(primarySlotTagName, props, [
primary: getNativeElementProps(primarySlotTagName, props, [
...(excludedPropNames || []),
'style',
'className',
]),
] as ExcludedPropKeys[]),
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,29 @@ import { getNativeProps, divProperties } from './properties';

describe('getNativeProps', () => {
it('can pass through data tags', () => {
const result = getNativeProps<React.HTMLAttributes<HTMLDivElement>>(
const result = getNativeProps(
{
'data-automation-id': 1,
},
divProperties,
);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
expect((result as any)['data-automation-id']).toEqual(1);

expect(result['data-automation-id']).toEqual(1);
});

it('can pass through aria tags', () => {
const result = getNativeProps<React.HTMLAttributes<HTMLDivElement>>(
{
'aria-label': 1,
'aria-label': '1',
},
divProperties,
);

expect(result['aria-label']).toEqual(1);
expect(result['aria-label']).toEqual('1');
});

it('can pass through basic div properties and events', () => {
//
const result = getNativeProps<React.HTMLAttributes<HTMLDivElement>>(
{
className: 'foo',
Expand All @@ -44,7 +45,7 @@ describe('getNativeProps', () => {
});

it('can remove unexpected properties', () => {
const result = getNativeProps<React.HTMLAttributes<HTMLDivElement>>(
const result = getNativeProps(
{
foobar: 1,
className: 'hi',
Expand All @@ -53,14 +54,14 @@ describe('getNativeProps', () => {
);

expect(result.className).toEqual('hi');
// eslint-disable-next-line @typescript-eslint/no-explicit-any
expect((result as any).foobar).toEqual(undefined);
expect(result.foobar).toEqual(undefined);
});

it('can exclude properties', () => {
const result = getNativeProps<{ a: number; b: number }>({ a: 1, b: 2 }, ['a', 'b'], ['b']);
const result = getNativeProps({ a: 1, b: 2 }, ['a', 'b'], ['b']);

expect(result.a).toBeDefined();
// @ts-expect-error -- strict type checking for exclusion, b doesn't exist after removal
expect(result.b).toBeUndefined();
});
});
Loading