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.

Sign up for GitHub

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

Jump to bottom

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

feat(eslint-plugin-react-components): add prefer-fluentui-v9 rule #33449

wants to merge 16 commits into from

Conversation

dmytrokirpa
Copy link
Contributor

Previous Behavior

--

New Behavior

  • Added new rule prefer-fluentui-v9 to eslint-plugin-react-components to enforce the use of Fluent UI v9 equivalent for Fluent UI v8 components.
  • Added it to the recommended config.
  • Updated docs

Example of output:

fluentui/apps/vr-tests/src/stories/Toggle.stories.tsx
  4:1  warning  Avoid importing Toggle from '@fluentui/react', as this package has already migrated to Fluent UI 9. Import Switch from '@fluentui/react-components' instead  @fluentui/react-components/prefer-fluentui-v9

fluentui/apps/vr-tests/src/stories/Tooltip/Tooltip.stories.tsx
  4:1  warning  Avoid importing TooltipHost from '@fluentui/react', as this package has already migrated to Fluent UI 9. Import Tooltip from '@fluentui/react-components' instead  @fluentui/react-components/prefer-fluentui-v9

Related Issue(s)

@dmytrokirpa dmytrokirpa requested review from a team December 11, 2024 14:45
@dmytrokirpa dmytrokirpa self-assigned this Dec 11, 2024
@dmytrokirpa dmytrokirpa requested a review from a team as a code owner December 11, 2024 14:45
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 11, 2024 •
edited
Loading

📊 Bundle size report

✅ No changes found

@dmytrokirpa
Copy link
Contributor Author

cc @NotWoods

@github-actions GitHub Actions
Copy link

Pull request demo site: URL

@dmytrokirpa dmytrokirpa requested review from NotWoods and mainframev and removed request for NotWoods December 12, 2024 11:16
@dmytrokirpa dmytrokirpa requested a review from Hotell December 30, 2024 17:05
@dmytrokirpa dmytrokirpa enabled auto-merge (squash) December 31, 2024 14:57
Hotell
Hotell approved these changes Jan 2, 2025
View reviewed changes
Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

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

some comments that need to be addressed as part of PR but others can be a follow-up. approving to unblock once the change file is tweaked.

stoked to have 1st rule being shipped soon !
ty

packages/react-components/eslint-plugin-react-components/src/index.ts
};

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.

packages/react-components/eslint-plugin-react-components/src/index.ts Show resolved Hide resolved
packages/react-components/eslint-plugin-react-components/src/rules/prefer-fluentui-v9.ts Outdated
defaultOptions: [],
create(context) {
return {
// eslint-disable-next-line @typescript-eslint/naming-convention
Copy link
Contributor

Choose a reason for hiding this comment

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

this goes in the way every time we wanna access AST, should we turn off this rule completely for rule implementation files ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

packages/react-components/eslint-plugin-react-components/tsconfig.spec.json
@@ -1,7 +1,8 @@
{
"extends": "./tsconfig.json",
"compilerOptions": {
"module": "CommonJS",
"module": "NodeNext",
Copy link
Contributor

Choose a reason for hiding this comment

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

type errors right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, typescript-eslint/utils doens't work well with the commonjs

packages/react-components/eslint-plugin-react-components/src/rules/prefer-fluentui-v9.ts Outdated
ToggleButton: 'ToggleButton',
Toggle: 'Switch',
Tooltip: 'Tooltip',
} 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

packages/react-components/eslint-plugin-react-components/src/rules/prefer-fluentui-v9.ts
* Migrations from Fluent 8 components to Fluent 9 components.
* @see https://react.fluentui.dev/?path=/docs/concepts-migration-from-v8-component-mapping--docs
*/
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.

packages/react-components/eslint-plugin-react-components/src/rules/prefer-fluentui-v9.ts
create(context) {
return {
// eslint-disable-next-line @typescript-eslint/naming-convention
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.

change/@fluentui-eslint-plugin-react-components-1c2bc691-c857-43cf-9806-ffe8d0d058c0.json Outdated
@@ -0,0 +1,7 @@
{
"type": "minor",
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 be breaking change , we use zero based versioning for now -> so any feat/fix = patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@dmytrokirpa dmytrokirpa disabled auto-merge January 2, 2025 13:43
@dmytrokirpa dmytrokirpa force-pushed the feat/add-eslint-rule-prefer-v9 branch from 6c824b5 to 0679d93 Compare January 2, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mainframev mainframev mainframev approved these changes

@NotWoods NotWoods NotWoods approved these changes

@Hotell Hotell Hotell approved these changes

Assignees

@dmytrokirpa dmytrokirpa

Labels
Fluent UI react-components (v9) Package: eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmytrokirpa @Hotell @NotWoods @mainframev