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: improve react-18 tests #25758

Merged
merged 7 commits into from
Nov 25, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
7 changes: 2 additions & 5 deletions apps/react-18-tests-v8/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
{
"extends": ["plugin:@fluentui/eslint-plugin/react--legacy"],
"extends": ["plugin:@fluentui/eslint-plugin/react"],
"root": true,
"rules": {
"deprecation/deprecation": "off",
"prefer-const": "off"
}
"rules": {}
}
11 changes: 10 additions & 1 deletion apps/react-18-tests-v8/jest.config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
// @ts-check

const { resolveMergeStylesSerializer } = require('@fluentui/scripts/jest/jest-resources');
const getResolveAlias = require('@fluentui/scripts/webpack/getResolveAlias');

/**
* @type {import('@jest/types').Config.InitialOptions}
*/
Expand All @@ -9,12 +12,18 @@ module.exports = {
globals: {
'ts-jest': {
tsConfig: '<rootDir>/tsconfig.spec.json',
diagnostics: false,
diagnostics: { warnOnly: true, exclude: ['packages/**'] },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this exposed lot of warnings from v8 packages - caused by not following isolateModules.

this will resolve it #25774

},
},
transform: {
'^.+\\.tsx?$': 'ts-jest',
},

coverageDirectory: './coverage',
setupFilesAfterEnv: ['./config/tests.js'],
moduleNameMapper: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

previous setup was using path mappings for v9 which is has no effect

'\\.(scss)$': '@fluentui/scripts/jest/jest-style-mock.js',
...getResolveAlias(),
},
snapshotSerializers: [resolveMergeStylesSerializer()],
};
23 changes: 10 additions & 13 deletions apps/react-18-tests-v8/package.json
Original file line number Diff line number Diff line change
@@ -1,30 +1,27 @@
{
"name": "@fluentui/react-18-tests-v8",
"description": "React 18 test application",
"version": "1.0.0",
"version": "8.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is needed as tooling checks version and tweaks its behaviours based on that ( cypress )

"private": true,
"scripts": {
"build": "just-scripts build",
"clean": "just-scripts clean",
"code-style": "just-scripts code-style",
"type-check": "tsc -b tsconfig.json",
"format": "prettier -w . --ignore-path ../.prettierignore",
"format:check": "yarn format -c",
"e2e": "cypress run --component",
"e2e:local": "cypress open --component",
"lint": "just-scripts lint",
"test": "just-scripts test",
"just": "just-scripts",
"start": "webpack-dev-server --mode=development --config=webpack.config.js",
"type-check": "tsc -b tsconfig.json"
"lint": "eslint --ext .js,.ts.,.tsx ./src",
"test": "jest --passWithNoTests",
"start": "webpack-dev-server --mode=development"
},
"devDependencies": {
"@fluentui/eslint-plugin": "*",
"@fluentui/scripts": "^1.0.0",
"@types/react": "18.0.14",
"@types/react-dom": "18.0.6",
"swc-loader": "^0.2.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

swc-loader is in root package version already - single version policy conformance

"@fluentui/scripts": "^1.0.0"
},
"dependencies": {
"@fluentui/react": "^8.103.0",
"@fluentui/react-hooks": "^8.6.14",
"@types/react": "18.0.14",
"@types/react-dom": "18.0.6",
"react": "18.2.0",
"react-dom": "18.2.0",
"tslib": "^2.1.0"
Expand Down
2 changes: 1 addition & 1 deletion apps/react-18-tests-v8/src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as React from 'react';
import { ThemeProvider, DefaultButton, PartialTheme, getTheme } from '@fluentui/react';
import { useBoolean } from '@fluentui/react-hooks';
import { ContextualMenuExample } from './components/index';
import { ContextualMenuExample } from './components';

// This app is here as a simple sandbox to render v8 controls inside of an React 18 environment.

Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import * as React from 'react';
import { mount } from '@cypress/react';

import { ContextualMenuExample } from './ContextualMenuExample';

const menuTriggerSelector = '[type="button"]';
const menuSelector = '[role="menu"]';

describe('ContextualMenu in React 18', () => {
it('renders ContextualMenu when trigger button is clicked', () => {
mount(
<React.StrictMode>
<ContextualMenuExample />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this uses implementation instead of having the same code on 3 places

</React.StrictMode>,
);

cy.get(menuTriggerSelector).click().get(menuSelector).should('be.visible');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import * as React from 'react';
import { render } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import { ContextualMenuExample } from './ContextualMenuExample';

describe('ContextualMenu in React 18', () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
const noop = () => {};

beforeEach(() => {
jest.spyOn(console, 'warn').mockImplementation(noop);
});

it('should render ContextualMenu when trigger button is clicked', () => {
const { getByRole, queryAllByRole } = render(
<React.StrictMode>
<ContextualMenuExample />
</React.StrictMode>,
);

expect(queryAllByRole('menu').length).toBe(0);

const menuTrigger = getByRole('button');
userEvent.click(menuTrigger);

expect(queryAllByRole('menu').length).toBe(1);
});
});
4 changes: 2 additions & 2 deletions apps/react-18-tests-v8/tsconfig.app.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
"declaration": true,
"declarationDir": "dist/types",
"inlineSources": true,
"types": ["static-assets", "environment", "node"]
"types": ["static-assets", "environment"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

node types should not be present in web app

},
"exclude": ["./src/common/**", "**/*.spec.ts", "**/*.spec.tsx", "**/*.test.ts", "**/*.test.tsx"],
"exclude": ["**/*.spec.ts", "**/*.spec.tsx", "**/*.test.ts", "**/*.test.tsx", "**/*.cy.ts", "**/*.cy.tsx"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to follow our approach with .cy

"include": ["./src/**/*.ts", "./src/**/*.tsx"]
}
4 changes: 2 additions & 2 deletions apps/react-18-tests-v8/tsconfig.cy.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
"compilerOptions": {
"jsx": "react",
"isolatedModules": false,
"types": ["node", "cypress", "cypress-storybook/cypress", "cypress-real-events"],
"types": ["node", "cypress", "cypress-real-events"],
"lib": ["ES2019", "dom"]
},
"include": ["./src/**/*.e2e.ts", "./src/**/*.e2e.tsx"]
"include": ["./src/**/*.cy.ts", "./src/**/*.cy.tsx"]
}
47 changes: 39 additions & 8 deletions apps/react-18-tests-v8/webpack.config.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const path = require('path');

const HtmlWebpackPlugin = require('html-webpack-plugin');
const { TsconfigPathsPlugin } = require('tsconfig-paths-webpack-plugin');

const getResolveAlias = require('@fluentui/scripts/webpack/getResolveAlias');

module.exports = () => {
return {
Expand All @@ -12,22 +12,53 @@ module.exports = () => {
},
resolve: {
extensions: ['.tsx', '.ts', '.js'],
plugins: [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this had no use as it would map only v9 packages

new TsconfigPathsPlugin({
configFile: path.resolve(__dirname, '../../tsconfig.base.json'),
}),
],
alias: {
react: path.resolve(__dirname, './node_modules/react'),
'react-dom': path.resolve(__dirname, './node_modules/react-dom'),
...getResolveAlias(),
},
},
module: {
rules: [
{
test: /\.tsx?$/,
exclude: /node_modules/,
use: 'swc-loader',
use: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

v8 cannot be compiled with swc, switching to esbuild for now swc-project/swc#2037

loader: 'esbuild-loader',
options: {
loader: 'tsx',
target: 'es2019',
},
},
},
{
test: /\.scss$/,
Copy link
Member

Choose a reason for hiding this comment

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

why are sass and css loaders only added now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

previously v8 needed to build everything prior to run. also it was misconfigured to use v9 path aliases etc. more info is in PR description

enforce: 'pre',
exclude: [/node_modules/],
use: [
{
loader: '@microsoft/loader-load-themed-styles', // creates style nodes from JS strings
},
{
loader: 'css-loader', // translates CSS into CommonJS
options: {
esModule: false,
modules: true,
importLoaders: 2,
},
},
{
loader: 'postcss-loader',
options: {
postcssOptions: {
plugins: ['autoprefixer'],
},
},
},
{
loader: 'sass-loader',
},
],
},
],
},
Expand Down
7 changes: 2 additions & 5 deletions apps/react-18-tests-v9/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
{
"extends": ["plugin:@fluentui/eslint-plugin/react--legacy"],
"extends": ["plugin:@fluentui/eslint-plugin/react"],
"root": true,
"rules": {
"deprecation/deprecation": "off",
"prefer-const": "off"
}
"rules": {}
}
18 changes: 18 additions & 0 deletions apps/react-18-tests-v9/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,21 @@ This file and process will be replaced with Storybook once we are able to get st
```

Add test files for React 18 issues that have been triaged and resolved so that we do not regress.

### `type-check`

To be able to type-check cross React versions we need all v9 libraries build up front so we don't type check all v9 implementation rather public API surface.

For that purpose we use `tsconfig.react-compat-check.json` as target for `type-check` npm script task, which disables path aliases and forces `tsc` to consume linked monorepo build packages.

**Local machine flow:**

```sh
lage build --to @fluentui/react-18-tests-v9

yarn workspace @fluentui/react-18-tests-v9 type-check
```

**CI:**

lage defines `build` targets to be executed prior to `type-check`.
Loading