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(eslint-plugin-react-components): add prefer-fluentui-v9 rule #33449

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
Hotell marked this conversation as resolved.
Show resolved Hide resolved
"comment": "feat: add prefer-fluentui-v9 rule",
"packageName": "@fluentui/eslint-plugin-react-components",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,46 @@ module.exports = {
};
```

1. Or configure individual rules manually:
2. Or configure individual rules manually:

```js
module.exports = {
plugins: ['@fluentui/react-components'],
rules: {
'@fluentui/react-components/rule-name-1': 'error',
'@fluentui/react-components/rule-name-2': 'warn',
'@fluentui/react-components/prefer-fluentui-v9': 'warn',
},
};
```

## Available Rules

TBD
### prefer-fluentui-v9

This rule ensures the use of Fluent UI v9 counterparts for Fluent UI v8 components.

#### Options

- `unstable` (boolean): Whether to enforce Fluent UI v9 preview component migrations.
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved

#### Examples

**✅ Do**

```js
// Import and use components that have been already migrated to Fluent UI v9
import { Button } from '@fluentui/react-components';

const Component = () => <Button>...</Button>;
```

**❌ Don't**

```js
// Avoid importing and using Fluent UI V8 components that have already been migrated to Fluent UI V9.
import { DefaultButton } from '@fluentui/react';

const Component = () => <DefaultButton>...</DefaultButton>;
```

## License

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,29 @@

```ts

import { RuleListener } from '@typescript-eslint/utils/dist/ts-eslint';
import { RuleModule } from '@typescript-eslint/utils/dist/ts-eslint';

// @public (undocumented)
const plugin: {
export const plugin: {
meta: {
name: string;
version: string;
};
configs: {
recommended: {
plugins: string[];
rules: {};
rules: {
"@fluentui/react-components/prefer-fluentui-v9": string;
};
};
};
rules: {};
rules: {
"prefer-fluentui-v9": RuleModule<"replaceFluent8With9" | "replaceFluent8With9Unstable" | "replaceIconWithJsx" | "replaceStackWithFlex", {
unstable?: boolean | undefined;
}[], unknown, RuleListener>;
};
};
export default plugin;

// (No @packageDocumentation comment for this package)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
import { name, version } from '../package.json';
import { RULE_NAME as preferFluentUIV9Name, rule as preferFluentUIV9 } from './rules/prefer-fluentui-v9';

const allRules = {
// add all rules here
[preferFluentUIV9Name]: preferFluentUIV9,
};

const configs = {
recommended: {
plugins: [name],
rules: {
// add all recommended rules here
[`@fluentui/react-components/${preferFluentUIV9Name}`]: 'warn',
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if this is good idea at this stage - any kind of modifications of config presets will be a breaking change for users. personally I'm fine having it as we properly follow semver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The preset usage is entirely optional. Consumers can always switch to individual rules if they prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah what I meant is scenario once someone adds recommended preset to application - and we introduce BC they will be affected ( with picked rules manually not so much )

},
},
};

// Plugin definition
const plugin = {
export const plugin = {
meta: {
name,
version,
Expand All @@ -33,4 +34,4 @@ Object.assign(configs, {
},
});

export default plugin;
module.exports = plugin;
Hotell marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { RuleTester } from '@typescript-eslint/rule-tester';
import { RULE_NAME, rule } from './prefer-fluentui-v9';

const ruleTester = new RuleTester();

ruleTester.run(RULE_NAME, rule, {
valid: [
{
code: `import type { IDropdownOption } from '@fluentui/react';`,
},
{
code: `import type { ITheme } from '@fluentui/react';`,
},
{
code: `import { ThemeProvider } from '@fluentui/react';`,
},
{
code: `import { Button } from '@fluentui/react-components';`,
},
{
code: `import { Rating } from '@fluentui/react';`,
options: [{ unstable: false }],
},
],
invalid: [
{
code: `import { Dropdown, Icon } from '@fluentui/react';`,
errors: [{ messageId: 'replaceFluent8With9' }, { messageId: 'replaceIconWithJsx' }],
},
{
code: `import { Stack } from '@fluentui/react';`,
errors: [{ messageId: 'replaceStackWithFlex' }],
},
{
code: `import { DatePicker } from '@fluentui/react';`,
errors: [{ messageId: 'replaceFluent8With9Unstable' }],
},
{
code: `import { Rating } from '@fluentui/react';`,
options: [{ unstable: true }],
errors: [{ messageId: 'replaceFluent8With9Unstable' }],
},
],
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
import { createRule } from './utils/create-rule';

export const RULE_NAME = 'prefer-fluentui-v9';

type Options = Array<{
/** Whether to enforce Fluent UI v9 preview component migrations. */
unstable?: boolean;
}>;

type MessageIds = 'replaceFluent8With9' | 'replaceFluent8With9Unstable' | 'replaceIconWithJsx' | 'replaceStackWithFlex';

export const rule = createRule<Options, MessageIds>({
name: RULE_NAME,
meta: {
type: 'problem',
docs: {
description: 'This rule ensures the use of Fluent UI v9 counterparts for Fluent UI v8 components.',
},
schema: [
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
{
type: 'object',
properties: {
unstable: { type: 'boolean' },
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
},
additionalProperties: false,
},
],
messages: {
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
replaceFluent8With9: `Avoid importing {{ fluent8 }} from '@fluentui/react', as this package has already migrated to Fluent UI 9. Import {{ fluent9 }} from '@fluentui/react-components' instead.`,
replaceFluent8With9Unstable: `Avoid importing {{ fluent8 }} from '@fluentui/react', as this package has started migration to Fluent UI 9. Import {{ fluent9 }} from '{{ package }}' instead.`,
replaceIconWithJsx: `Avoid using Icon from '@fluentui/react', as this package has already migrated to Fluent UI 9. Use a JSX SVG icon from '@fluentui/react-icons' instead.`,
replaceStackWithFlex: `Avoid using Stack from '@fluentui/react', as this package has already migrated to Fluent UI 9. Use native CSS flexbox instead.`,
},
},
defaultOptions: [],
create(context) {
const { unstable = false } = context.options[0] ?? {};

return {
// eslint-disable-next-line @typescript-eslint/naming-convention
Hotell marked this conversation as resolved.
Show resolved Hide resolved
ImportDeclaration(node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this will not work for lazy imports ( not sure how often that appears in user land with fluent but we should add it as a follow-up)

const lazyComponents = await import('@fluentui/react');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the example you gave, it's tough to do a static analysis to figure out which v8 components are used and suggest v9 alternatives. It might be doable, but I'm not sure if it's worth the effort. We could add a warning with a generic message, but I'm not sure how useful that would be.

Copy link
Contributor

Choose a reason for hiding this comment

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

implementation/effort wise it's +- same logic with additional parse of destructured symbol but for different AST query selector. There is no additional complexity behind it.

users might use lazy loading modules in any kind of Suspense context or for lazy loaded application routes for perf reasons.

example: calendar is non trivial component having huge bundle size impact, in application context you don't wanna have it part of default bundle.

i think it is worth it to cover this scenario. Regarding internal users of v8 and lazy loading we need more data indeed.

if (node.source.value !== '@fluentui/react') {
return;
}

for (const specifier of node.specifiers) {
if (specifier.type === 'ImportSpecifier' && specifier.imported.type === 'Identifier') {
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
const name = specifier.imported.name;

switch (name) {
case 'Icon':
context.report({ node, messageId: 'replaceIconWithJsx' });
break;
case 'Stack':
context.report({ node, messageId: 'replaceStackWithFlex' });
break;
default:
if (isMigration(name)) {
context.report({
node,
messageId: 'replaceFluent8With9',
data: {
fluent8: name,
fluent9: MIGRATIONS[name],
},
});
} else if (isCompatMigration(name)) {
context.report({
node,
messageId: 'replaceFluent8With9Unstable',
data: {
fluent8: name,
fluent9: COMPAT_MIGRATIONS[name].component,
package: COMPAT_MIGRATIONS[name].package,
},
});
} else if (isPreviewMigration(name) && unstable) {
context.report({
node,
messageId: 'replaceFluent8With9Unstable',
data: {
fluent8: name,
fluent9: PREVIEW_MIGRATIONS[name].component,
package: PREVIEW_MIGRATIONS[name].package,
},
});
}
break;
}
}
}
},
};
},
});

type Migration = string | { component: string; package: string };

/**
* Migrations from Fluent 8 components to Fluent 9 components as of Fluent UI 9.46.3
* @see https://react.fluentui.dev/?path=/docs/concepts-migration-from-v8-component-mapping--page
*/
const MIGRATIONS = {
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if we could generate this list by automation and update when we trigger stable release for v9 controll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can consider adding components to the map during the transition from preview to stable. However, we still need to define the relationship between the v8 and v9 components.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah some json metadata file per project sounds like good pattern ( which can be than leveraged for any kind of automations like this - I started implementing something like that for triage automation, will share more info later )

mergeStyles: 'makeStyles',
ActionButton: 'Button',
Announced: 'useAnnounce',
Breadcrumb: 'Breadcrumb',
CommandBar: 'Toolbar',
CommandBarButton: 'ToolbarButton',
CommandButton: 'MenuButton',
CompoundButton: 'CompoundButton',
Callout: 'Popover',
Checkbox: 'Checkbox',
ChoiceGroup: 'Radio',
ComboBox: 'Dropdown',
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
ContextualMenu: 'Menu',
DefaultButton: 'Button',
DetailsList: 'DataGrid',
Dialog: 'Dialog',
DocumentCard: 'Card',
Dropdown: 'Dropdown',
Facepile: 'AvatarGroup',
GroupedList: 'Tree',
IconButton: 'Button',
Image: 'Image',
Label: 'Label',
Layer: 'Portal',
Link: 'Link',
MessageBar: 'MessageBar',
Modal: 'Dialog',
OverflowSet: 'Overflow',
Overlay: 'Portal',
Panel: 'Drawer',
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
Popup: 'Dialog',
PrimaryButton: 'Button',
Persona: 'Avatar',
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
Pivot: 'TabList',
PivotItem: 'Tab',
ProgressIndicator: 'ProgressBar',
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
Separator: 'Divider',
Shimmer: 'Skeleton',
Slider: 'Slider',
SplitButton: 'SplitButton',
SpinButton: 'SpinButton',
Spinner: 'Spinner',
Text: 'Text',
TextField: 'Input',
ToggleButton: 'ToggleButton',
Toggle: 'Switch',
Tooltip: 'Tooltip',
TooltipHost: 'Tooltip',
VerticalDivider: 'Divider',
} satisfies Record<string, Migration>;

/**
* Compatibility migrations for certain components.
* @see https://react.fluentui.dev/?path=/docs/concepts-migration-from-v8-component-mapping--page
*/
const COMPAT_MIGRATIONS = {
DatePicker: { component: 'DatePicker', package: '@fluentui/react-datepicker-compat' },
TimePicker: { component: 'TeachingPopover', package: '@fluentui/react-timepicker-compat' },
} satisfies Record<string, Migration>;

/**
* Preview migrations for certain components.
* @see https://react.fluentui.dev/?path=/docs/concepts-migration-from-v8-component-mapping--page
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
*/
const PREVIEW_MIGRATIONS = {
Rating: { component: 'Rating', package: '@fluentui/react-rating-preview' },
dmytrokirpa marked this conversation as resolved.
Show resolved Hide resolved
TeachingBubble: { component: 'TeachingPopover', package: '@fluentui/react-teaching-popover-preview' },
} satisfies Record<string, Migration>;
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: it would be probably better to use 1 Schema ( more verbose ) so every value has component and package.

that would remove need of getMigrationDetails completely


/**
* Checks if a component name is in the MIGRATIONS list.
* @param name - The name of the component.
* @returns True if the component is in the MIGRATIONS list, false otherwise.
*/
const isMigration = (name: string): name is keyof typeof MIGRATIONS => name in MIGRATIONS;

/**
* Checks if a component name is in the COMPAT_MIGRATIONS list.
* @param name - The name of the component.
* @returns True if the component is in the COMPAT_MIGRATIONS list, false otherwise.
*/
const isCompatMigration = (name: string): name is keyof typeof COMPAT_MIGRATIONS => name in COMPAT_MIGRATIONS;

/**
* Checks if a component name is in the PREVIEW_MIGRATIONS list.
* @param name - The name of the component.
* @returns True if the component is in the PREVIEW_MIGRATIONS list, false otherwise.
*/
const isPreviewMigration = (name: string): name is keyof typeof PREVIEW_MIGRATIONS => name in PREVIEW_MIGRATIONS;
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { ESLintUtils } from '@typescript-eslint/utils';

/**
* Creates an ESLint rule with a pre-configured URL pointing to the rule's documentation.
*/
export const createRule = ESLintUtils.RuleCreator(
name =>
`https://github.com/microsoft/fluentui/blob/master/packages/react-components/eslint-plugin-react-components/README.md#${name}`,
);
Loading