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

chore: implement test in memory execution based on TS aliases #17161

Merged
merged 7 commits into from
Mar 11, 2021
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ coverage
dist
dist-storybook
screenshots
.cache

*.tar.gz

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "chore(make-styles): propagate deps to root and remove path overrides so the package can be properly consumed by ts path aliases converged packages",
"packageName": "@fluentui/make-styles",
"email": "[email protected]",
"dependentChangeType": "none"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "fix(react-conformance): remove esModuleInterop so the package is compliant with other packages",
"packageName": "@fluentui/react-conformance",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "chore(react-menu): implement in memory test exection",
"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(utilities): make pkg compile in strict mode",
"packageName": "@fluentui/utilities",
"email": "[email protected]",
"dependentChangeType": "none"
}
3 changes: 3 additions & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = {
projects: ['<rootDir>/packages/react-menu'],
};
28 changes: 28 additions & 0 deletions jest.preset.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// @ts-check

const path = require('path');
const { pathsToModuleNameMapper } = require('ts-jest/utils');

const tsConfig = require('./tsconfig.base.json');

const tsPathAliases = pathsToModuleNameMapper(tsConfig.compilerOptions.paths, {
prefix: `<rootDir>/${path.relative(process.cwd(), __dirname)}/`,
});

/**
* @type {jest.InitialOptions}
*/
const baseConfig = {
transform: {
'^.+\\.ts$': 'ts-jest',
Copy link
Member

Choose a reason for hiding this comment

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

Why not include tsx in the transform by default, since it's so commonly used? Doesn't seem like it should hurt anything to have it listed here in packages that don't use tsx. (In general, if there are changes like this that we can make to reduce the number of manual overrides per package, I would very much prefer that in the interest of reducing maintenance headaches of having config duplicated tons of places.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to mitigate anyone to create *.test.tsx test files by "accident" for non react packages

Copy link
Member

Choose a reason for hiding this comment

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

I see the point, but how bad is that? To me it's not great, but I'd prefer the relatively low risk (and low consequence) of that occurring over the maintenance headaches that come from keeping a duplicated config in a whole bunch of places. (Not blocking on this though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

occurring over the maintenance headaches

I see your concern. this is easily mitigated with existing oss tooling there is almost none, as everything can be updated/tweaked by proper migration/update scripts

thought: ideally I'd like us to get rid of ts-jest and use babel/esbuild for everything, but that's out off scope for this PR

},
testMatch: ['**/+(*.)+(spec|test).+(ts|js)?(x)'],
moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx'],
testPathIgnorePatterns: ['/node_modules/', '/lib/', '/lib-commonjs/', '/dist/'],
moduleNameMapper: { ...tsPathAliases },
cacheDirectory: '<rootDir>/.cache/jest',
clearMocks: true,
watchPlugins: ['jest-watch-typeahead/filename', 'jest-watch-typeahead/testname'],
};

module.exports = { ...baseConfig };
Copy link
Member

Choose a reason for hiding this comment

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

Not very important but why the spread?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

extensibility without less git diff noise.

my thinking was to keep it like this -> extract common preset to separate package with tests etc (not within scripts/), then just use api from that package and leave room for any tweaks additions in here without noisy git diffs

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, by "common preset" do you mean just the tsconfig or all shared elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

module.exports = {
 preset: '@fluentui/jest-preset'
}

or

// for more fine grained controll
const {baseConfig} = require('@fluentui/jest-preset');

module.exports = {
  ...baseConfig,
  // custom override
  clearMocks: false
}

6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@
"postcss-loader": "4.1.0",
"postcss-modules": "2.0.0",
"ts-loader": "8.0.14",
"ts-jest": "24.0.1",
"ts-jest": "24.3.0",
"typescript": "3.7.2",
"webpack-dev-server": "4.0.0-beta.0",
"webpack-cli": "4.3.1",
Expand All @@ -124,6 +124,10 @@
"packages/fluentui/*"
],
"nohoist": [
"@fluentui/make-styles/stylis",
"@fluentui/make-styles/@types/stylis",
"@fluentui/react-bindings/stylis",
"@fluentui/react-northstar-fela-renderer/stylis",
"packages/web-components/webpack",
"packages/web-components/@storybook/**",
"packages/web-components/typescript",
Expand Down
2 changes: 1 addition & 1 deletion packages/make-styles/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"@fluentui/eslint-plugin": "^1.0.1",
"@fluentui/scripts": "^1.0.0",
"@fluentui/test-utilities": "^8.0.1",
"@types/stylis": "^4.0.0"
"@types/stylis": "4.0.0"
},
"dependencies": {
"@emotion/hash": "^0.8.0",
Expand Down
4 changes: 0 additions & 4 deletions packages/make-styles/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{
"compilerOptions": {
"baseUrl": ".",
"outDir": "dist",
"target": "es5",
"module": "commonjs",
Expand All @@ -15,9 +14,6 @@
"moduleResolution": "node",
"preserveConstEnums": true,
"lib": ["es5", "dom"],
"paths": {
"stylis": ["../../node_modules/@types/stylis"]
},
"typeRoots": ["../../node_modules/@types", "../../typings"],
"types": ["custom-global", "jest", "inline-style-expand-shorthand"]
},
Expand Down
2 changes: 1 addition & 1 deletion packages/react-conformance/src/defaultErrorMessages.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { IsConformantOptions } from './types';
import { ComponentDoc } from 'react-docgen-typescript';

import chalk from 'chalk';
import os from 'os';
import * as os from 'os';
import parseDocblock from './utils/parseDocblock';

import * as _ from 'lodash';
Expand Down
2 changes: 1 addition & 1 deletion packages/react-conformance/src/isConformant.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { IsConformantOptions } from './types';
import { defaultTests } from './defaultTests';
import { defaultErrorMessages } from './defaultErrorMessages';
import { merge } from './utils/merge';
import os from 'os';
import * as os from 'os';
import chalk from 'chalk';
import { getComponentDoc } from './utils/getComponentDoc';

Expand Down
1 change: 0 additions & 1 deletion packages/react-conformance/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
"strict": true,
"moduleResolution": "node",
"preserveConstEnums": true,
"esModuleInterop": true,
"lib": ["es2017", "dom"],
"types": ["jest", "node"]
},
Expand Down
27 changes: 19 additions & 8 deletions packages/react-menu/jest.config.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
const { createConfig, resolveMergeStylesSerializer } = require('@fluentui/scripts/jest/jest-resources');
const path = require('path');
// @ts-check

const config = createConfig({
setupFiles: [path.resolve(path.join(__dirname, 'config', 'tests.js'))],
snapshotSerializers: [resolveMergeStylesSerializer()],
});

module.exports = config;
/**
* @type {jest.InitialOptions}
*/
module.exports = {
displayName: 'react-menu',
preset: '../../jest.preset.js',
globals: {
'ts-jest': {
tsConfig: '<rootDir>/tsconfig.json',
diagnostics: false,
},
},
transform: {
'^.+\\.tsx?$': 'ts-jest',
},
coverageDirectory: './coverage',
setupFilesAfterEnv: ['./config/tests.js'],
};
6 changes: 3 additions & 3 deletions packages/react-menu/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
"just": "just-scripts",
"lint": "just-scripts lint",
"start": "just-scripts dev:storybook",
"start-test": "just-scripts jest-watch",
"test": "just-scripts test",
"update-snapshots": "just-scripts jest -u"
"start-test": "echo \"This is DEPRECATED instead use 'test --watch'\" && just-scripts jest-watch",
"test": "jest",
"update-snapshots": "echo \"This is DEPRECATED instead use 'test -u'\" && just-scripts jest -u"
},
"devDependencies": {
"@fluentui/eslint-plugin": "^1.0.1",
Expand Down
2 changes: 1 addition & 1 deletion packages/react-menu/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"importHelpers": true,
"noUnusedLocals": true,
"preserveConstEnums": true,
"types": ["jest", "custom-global"]
"types": ["jest", "custom-global", "inline-style-expand-shorthand"]
},
"include": ["src"]
}
2 changes: 1 addition & 1 deletion packages/utilities/etc/utilities.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export function asAsync<TProps>(options: IAsAsyncOptions<TProps>): React.Forward
export function assertNever(x: never): never;

// @public
export function assign(target: any, ...args: any[]): any;
export function assign(this: any, target: any, ...args: any[]): any;

// @public
export class Async {
Expand Down
6 changes: 3 additions & 3 deletions packages/utilities/src/AutoScroll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ export class AutoScroll {
private _events: EventGroup;
private _scrollableParent: HTMLElement | null;
private _scrollRect: IRectangle | undefined;
private _scrollVelocity: number;
private _isVerticalScroll: boolean;
private _timeoutId: number;
private _scrollVelocity!: number;
private _isVerticalScroll!: boolean;
private _timeoutId!: number;

constructor(element: HTMLElement) {
this._events = new EventGroup(this);
Expand Down
2 changes: 1 addition & 1 deletion packages/utilities/src/BaseComponent.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ describe('BaseComponent', () => {
it('can resolve refs', () => {
// eslint-disable-next-line deprecation/deprecation
class Foo extends BaseComponent<{}, {}> {
public root: HTMLElement;
public root!: HTMLElement;

public render(): JSX.Element {
// eslint-disable-next-line deprecation/deprecation
Expand Down
12 changes: 6 additions & 6 deletions packages/utilities/src/BaseComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ export class BaseComponent<TProps extends IBaseProps = {}, TState = {}> extends
* implementing a passthrough (higher-order component), you would set this to false and pass through
* the props to the inner component, allowing it to resolve the componentRef.
*/
protected _skipComponentRefResolution: boolean;
protected _skipComponentRefResolution!: boolean;

private __async: Async;
private __events: EventGroup;
private __disposables: IDisposable[] | null;
private __resolves: { [name: string]: (ref: React.ReactNode) => React.ReactNode };
private __className: string;
private __async!: Async;
private __events!: EventGroup;
private __disposables!: IDisposable[] | null;
private __resolves!: { [name: string]: (ref: React.ReactNode) => React.ReactNode };
private __className!: string;

/**
* BaseComponent constructor
Expand Down
8 changes: 7 additions & 1 deletion packages/utilities/src/EventGroup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export class EventGroup {
private _parent: any;
private _eventRecords: IEventRecord[];
private _id: number = EventGroup._uniqueId++;
private _isDisposed: boolean;
private _isDisposed!: boolean;

/** For IE8, bubbleEvent is ignored here and must be dealt with by the handler.
* Events raised here by default have bubbling set to false and cancelable set to true.
Expand All @@ -80,6 +80,8 @@ export class EventGroup {
target.fireEvent('on' + eventName, evObj);
}
} else {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore -- FIXME: strictBindCallApply error - https://github.com/microsoft/fluentui/issues/17331
while (target && retVal !== false) {
let events = <IEventRecordsByName>target.__events__;
let eventRecords = events ? events[eventName] : null;
Expand All @@ -89,6 +91,8 @@ export class EventGroup {
if (eventRecords.hasOwnProperty(id)) {
let eventRecordList = <IEventRecord[]>eventRecords[id];

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore -- FIXME: strictBindCallApply error - https://github.com/microsoft/fluentui/issues/17331
for (let listIndex = 0; retVal !== false && listIndex < eventRecordList.length; listIndex++) {
let record = eventRecordList[listIndex];

Expand Down Expand Up @@ -206,6 +210,8 @@ export class EventGroup {
let result;
try {
result = callback.apply(parent, args);
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore -- FIXME: strictBindCallApply error - https://github.com/microsoft/fluentui/issues/17331
if (result === false && args[0]) {
let e = args[0];

Expand Down
2 changes: 1 addition & 1 deletion packages/utilities/src/appendFunction.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('appendFunction', () => {
});

it('preserves the parent', () => {
function add(): void {
function add(this: { counter: number }): void {
this.counter++;
}

Expand Down
9 changes: 6 additions & 3 deletions packages/utilities/src/initializeComponentRef.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,22 @@ export function initializeComponentRef<TProps extends IBaseProps, TState>(obj: R
});
}

function _onMount(): void {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function _onMount(this: any): void {
_setComponentRef(this.props.componentRef, this);
}

function _onUpdate(prevProps: IBaseProps): void {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function _onUpdate(this: any, prevProps: IBaseProps): void {
if (prevProps.componentRef !== this.props.componentRef) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
_setComponentRef((prevProps as any).componentRef, null);
_setComponentRef(this.props.componentRef, this);
}
}

function _onUnmount(): void {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function _onUnmount(this: any): void {
_setComponentRef(this.props.componentRef, null);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/utilities/src/object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export function shallowCompare<TA extends any, TB extends any>(a: TA, b: TB): bo
* @returns Resulting merged target.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function assign(target: any, ...args: any[]): any {
export function assign(this: any, target: any, ...args: any[]): any {
return filteredAssign.apply(this, [null, target].concat(args));
}

Expand Down
22 changes: 11 additions & 11 deletions packages/utilities/src/selection/Selection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,24 @@ export type ISelectionOptionsWithRequiredGetKey<TItem> = ISelectionOptions<TItem
*/
export class Selection<TItem = IObjectWithKey> implements ISelection<TItem> {
/** Number of items selected. Do not modify. */
public count: number;
public readonly mode: SelectionMode;
public count!: number;
public readonly mode!: SelectionMode;

private _getKey: (item: TItem, index?: number) => string | number;
private _canSelectItem: (item: TItem, index?: number) => boolean;
private _getKey!: (item: TItem, index?: number) => string | number;
private _canSelectItem!: (item: TItem, index?: number) => boolean;

private _changeEventSuppressionCount: number;
private _items: TItem[];
private _selectedItems: TItem[] | null;
private _items!: TItem[];
private _selectedItems!: TItem[] | null;
private _selectedIndices: number[] | undefined;
private _isAllSelected: boolean;
private _exemptedIndices: { [index: string]: boolean };
private _isAllSelected!: boolean;
private _exemptedIndices!: { [index: string]: boolean };
private _exemptedCount: number;
private _keyToIndexMap: { [key: string]: number };
private _keyToIndexMap!: { [key: string]: number };
private _anchoredIndex: number;
private _onSelectionChanged: (() => void) | undefined;
private _hasChanged: boolean;
private _unselectableIndices: { [index: string]: boolean };
private _hasChanged!: boolean;
private _unselectableIndices!: { [index: string]: boolean };
private _unselectableCount: number;
private _isModal: boolean;

Expand Down
4 changes: 2 additions & 2 deletions packages/utilities/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
"jsx": "react",
"declaration": true,
"sourceMap": true,
"strict": true,
Copy link
Member

Choose a reason for hiding this comment

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

Since @fluentui/utilities no longer needs to be included in converged compilation, I'd prefer to revert these settings and the related changes.

Copy link
Contributor Author

@Hotell Hotell Mar 2, 2021

Choose a reason for hiding this comment

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

Im gonna keep it. I don't see reason to throw out work already done, as this API change will cause no harm. Looks like until now consumers of that API had any all over the place anyway, this will be just explicit. WDYT?

Copy link
Member

@layershifter layershifter Mar 3, 2021

Choose a reason for hiding this comment

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

I don't think that we should throw out any work, but at the same it's not anymore required for this PR 🤔

I propose to apply this changes in a separate PR (really want to have packages that have "strict" enabled ❤️ ). @ecraig12345 @Hotell what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

The main reason I was proposing reverting for now was in favor of a more detailed pass through to enable the option later (if we decide to invest in that). This would involve looking through the files and where possible, trying to understand the correct typing based on usage (and fix any missing null checks etc), rather than making the quickest change to silence the error.

I also very very strongly prefer to avoid @ts-ignore whenever possible since it's a "nuclear option": it turns off all type checking for the next line, rather than just the part where the error is coming from, opening up to more errors in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you already spent time reviewing those changes, there are only 2 ts-ignores documented ( Im gonna create issue and ref it there as well) which don't affect any public facing API.

I explicitly checked those ! non constructor assignment in affected files, to verify if that's good enough solution for now.

To summarise, while I can create another PR , I don't think it's necessary (reasoning provided). As a side note/thought, this PR is open for almost 2 weeks which is blocking the whole improved DX initiative for convergence ✌️

"strictFunctionTypes": false,
"experimentalDecorators": true,
"importHelpers": true,
"noImplicitAny": true,
"noUnusedLocals": true,
"strictNullChecks": true,
"forceConsistentCasingInFileNames": true,
"moduleResolution": "node",
"preserveConstEnums": true,
Expand Down
10 changes: 7 additions & 3 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@
"typeRoots": ["node_modules/@types", "./typings"],
"baseUrl": ".",
"paths": {
"@fluentui/set-version": ["packages/set-version/src/index.ts"],
"@fluentui/react-conformance": ["packages/react-conformance/src/index.ts"],
"@fluentui/react-context-selector": ["packages/react-context-selector/src/index.ts"],
"@fluentui/react-utilities": ["packages/react-utilities/src/index.ts"],
"@fluentui/react-make-styles": ["packages/react-make-styles/src/index.ts"],
"@fluentui/make-styles": ["packages/make-styles/src/index.ts"],
"@fluentui/react-provider": ["packages/react-provider/src/index.ts"],
"@fluentui/react-theme": ["packages/react-theme/src/index.ts"],
"@fluentui/react-theme-provider": ["packages/react-theme-provider/src/index.ts"],
"@fluentui/keyboard-key": ["packages/keyboard-key/src/index.ts"],
"@fluentui/react-menu": ["packages/react-menu/src/index.ts"],
"@fluentui/react-focus-management": ["packages/react-focus-management/src/index.ts"]
"@fluentui/react-focus-management": ["packages/react-focus-management/src/index.ts"],
"@fluentui/react-menu": ["packages/react-menu/src/index.ts"]
}
},
"exclude": ["node_modules"]
Expand Down
Loading