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

fix(reactant-module): fix DI performance #137

Merged
merged 3 commits into from
Sep 1, 2024
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
26 changes: 26 additions & 0 deletions packages/reactant-module/src/core/createStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,32 @@ export function createStore<T = any>({
}
}
const multipleInjectMap = getMetadata(METADATA_KEY.multiple);
// it's just a workaround for the issue of `ServiceIdentifiers` order.
// InversifyJS issue: https://github.com/inversify/InversifyJS/issues/1578
const allServiceIdentifiers = Array.from(ServiceIdentifiers);
allServiceIdentifiers.sort(([a], [b]) => {
let aDeps = [];
try {
aDeps = Reflect.getMetadata(METADATA_KEY.paramtypes, a) ?? [];
} catch (e) {
//
}
let bDeps = [];
try {
bDeps = Reflect.getMetadata(METADATA_KEY.paramtypes, b) ?? [];
} catch (e) {
//
}
return aDeps.length - bDeps.length;
ZouYouShun marked this conversation as resolved.
Show resolved Hide resolved
});
ServiceIdentifiers.clear();
// ServiceIdentifiers is sorted by the number of dependencies.
// The purpose is to ensure that the module is loaded in the correct order.
// The module with fewer dependencies is loaded first.
// ServiceIdentifiers is a mutable object, so it needs to be redefined.
allServiceIdentifiers.forEach(([ServiceIdentifier, value]) => {
ServiceIdentifiers.set(ServiceIdentifier, value);
});
for (const [ServiceIdentifier] of ServiceIdentifiers) {
// `Service` should be bound before `createStore`.
const isMultipleInjection = multipleInjectMap.has(ServiceIdentifier);
Expand Down
6 changes: 3 additions & 3 deletions packages/reactant-module/test/core/getRef.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,16 @@ test('getRef for base module', () => {
expect(counter.count).toBe(0);
expect(counter.string).toBe('test');
expect(Object.values(store.getState())).toEqual([
{ count: 0, others: { list: [] } },
{ list: [] },
{ count: 0, others: { list: [] } },
]);
counter.increase();
expect(counter.count).toBe(1);
counter.add();
expect(counter.others.list).toEqual([0]);
expect(Object.values(store.getState())).toEqual([
{ count: 1, others: { list: [0] } },
{ list: [] },
{ count: 1, others: { list: [0] } },
]);
expect((counter as any)[initStateKey].count).toBe(0);
expect(getRef(counter).initState!.count).toBe(0);
Expand All @@ -140,7 +140,6 @@ test('getRef for base module', () => {
(name) => !/^@@reactant/.test(name)
)
).toEqual([
'counter',
'string',
'number',
'symbol',
Expand All @@ -150,5 +149,6 @@ test('getRef for base module', () => {
'array',
'set',
'map',
'counter',
]);
});
14 changes: 7 additions & 7 deletions packages/reactant-module/test/decorators/computed.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe('@computed', () => {
});
expect(computedFn.mock.calls.length).toBe(0);
counter.increase();
expect(Object.values(store.getState())[0]).toEqual({ count: 1 });
expect(Object.values(store.getState())[1]).toEqual({ count: 1 });
expect(counter.num).toBe(2);
expect(computedFn.mock.calls.length).toBe(1);
expect(counter.num).toBe(2);
Expand Down Expand Up @@ -977,7 +977,7 @@ describe('@computed with automatic dependencies collection', () => {
expect(computedFn.mock.calls.length).toBe(0);
expect(computedFn0.mock.calls.length).toBe(1);
counter.increase();
expect(Object.values(store.getState())[0]).toEqual({ count: 1 });
expect(Object.values(store.getState())[1]).toEqual({ count: 1 });
expect(counter.num).toBe(3);
expect(computedFn.mock.calls.length).toBe(1);
expect(computedFn0.mock.calls.length).toBe(2);
Expand Down Expand Up @@ -1093,7 +1093,7 @@ describe('@computed with automatic dependencies collection', () => {
expect(computedFn0.mock.calls.length).toBe(1);
expect(computedFn1.mock.calls.length).toBe(1);
counter.increase();
expect(Object.values(store.getState())[0]).toEqual({ count: 1 });
expect(Object.values(store.getState())[1]).toEqual({ count: 1 });
expect(counter.num).toBe(3);
expect(computedFn.mock.calls.length).toBe(1);
expect(computedFn0.mock.calls.length).toBe(2);
Expand Down Expand Up @@ -1229,7 +1229,7 @@ describe('@computed with automatic dependencies collection', () => {
});
expect(computedFn.mock.calls.length).toBe(0);
counter.increase();
expect(Object.values(store.getState())[0]).toEqual({ count: 1 });
expect(Object.values(store.getState())[1]).toEqual({ count: 1 });
expect(counter.num).toBe(3);
expect(computedFn.mock.calls.length).toBe(1);
expect(counter.num).toBe(3);
Expand Down Expand Up @@ -1309,7 +1309,7 @@ describe('@computed with automatic dependencies collection', () => {
});
expect(computedFn.mock.calls.length).toBe(0);
counter.increase();
expect(Object.values(store.getState())[0]).toEqual({ count: 1 });
expect(Object.values(store.getState())[1]).toEqual({ count: 1 });
expect(counter.num).toBe(2);
expect(computedFn.mock.calls.length).toBe(1);
expect(counter.num).toBe(2);
Expand Down Expand Up @@ -1384,7 +1384,7 @@ describe('@computed with automatic dependencies collection', () => {
});
expect(computedFn.mock.calls.length).toBe(0);
counter.increase();
expect(Object.values(store.getState())[0]).toEqual({ count: 1 });
expect(Object.values(store.getState())[1]).toEqual({ count: 1 });
expect(counter.num).toBe(2);
expect(computedFn.mock.calls.length).toBe(1);
expect(counter.num).toBe(2);
Expand Down Expand Up @@ -1887,7 +1887,7 @@ describe('@computed with automatic dependencies collection', () => {
expect(computedFn0.mock.calls.length).toBe(0);
expect(counter.num).toBe(1);
counter.increase();
expect(Object.values(store.getState())[0]).toEqual({ count: 1 });
expect(Object.values(store.getState())[1]).toEqual({ count: 1 });
expect(counter.num).toBe(2);
expect(counter.counter0.num).toBe(1);
expect(computedFn0.mock.calls.length).toBe(1);
Expand Down
4 changes: 2 additions & 2 deletions packages/reactant-module/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,16 @@ test('base module with @state and @action', () => {
expect(counter.count).toBe(0);
expect(counter.string).toBe('test');
expect(Object.values(store.getState())).toEqual([
{ count: 0, others: { list: [] } },
{ list: [] },
{ count: 0, others: { list: [] } },
]);
counter.increase();
expect(counter.count).toBe(1);
counter.add();
expect(counter.others.list).toEqual([0]);
expect(Object.values(store.getState())).toEqual([
{ count: 1, others: { list: [0] } },
{ list: [] },
{ count: 1, others: { list: [0] } },
]);
expect((counter as any)[initStateKey].count).toBe(0);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,16 @@ test('base module for reduxDevToolsCompose', () => {
expect(counter.count).toBe(0);
expect(counter.string).toBe('test');
expect(Object.values(store.getState())).toEqual([
{ count: 0, others: { list: [] } },
{ list: [] },
{ count: 0, others: { list: [] } },
]);
counter.increase();
expect(counter.count).toBe(1);
counter.add();
expect(counter.others.list).toEqual([0]);
expect(Object.values(store.getState())).toEqual([
{ count: 1, others: { list: [0] } },
{ list: [] },
{ count: 1, others: { list: [0] } },
]);
expect(fn.mock.calls.length).toBe(1);
expect(JSON.parse(JSON.stringify(fn.mock.calls, null, 2))).toEqual([
Expand Down Expand Up @@ -212,15 +212,15 @@ test('base module with error for reduxDevToolsCompose', () => {
expect(counter.count).toBe(0);
expect(counter.string).toBe('test');
expect(Object.values(store.getState())).toEqual([
{ count: 0, others: { list: [] } },
{ list: [] },
{ count: 0, others: { list: [] } },
]);
counter.increase();
expect(counter.count).toBe(1);
counter.add();
expect(counter.others.list).toEqual([0]);
expect(Object.values(store.getState())).toEqual([
{ count: 1, others: { list: [0] } },
{ list: [] },
{ count: 1, others: { list: [0] } },
]);
});
3 changes: 2 additions & 1 deletion packages/reactant-router/src/router.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable consistent-return */
import React, { PropsWithChildren, FunctionComponent } from 'react';
import { PluginModule, injectable, inject, storeKey } from 'reactant-module';
import type { ReducersMapObject, Store } from 'redux';
import type { ReducersMapObject } from 'redux';
import {
connectRouter,
ConnectedRouter,
Expand All @@ -19,6 +19,7 @@ export {
} from 'history';

export { LOCATION_CHANGE } from 'connected-react-router';
export type { LocationChangeAction } from 'connected-react-router';

const RouterOptions = Symbol('RouterOptions');

Expand Down
53 changes: 49 additions & 4 deletions packages/reactant-share/src/modules/router.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable prefer-destructuring */
/* eslint-disable no-shadow */
/* eslint-disable @typescript-eslint/no-explicit-any */
/* eslint-disable consistent-return */
Expand All @@ -10,9 +11,14 @@ import {
stateKey,
modulesKey,
} from 'reactant';
import { Router as BaseReactantRouter, RouterOptions } from 'reactant-router';
import {
Router as BaseReactantRouter,
LOCATION_CHANGE,
RouterOptions,
} from 'reactant-router';
import type {
IRouterOptions as IBaseRouterOptions,
LocationChangeAction,
RouterState,
} from 'reactant-router';
import type { LocationState } from 'history';
Expand Down Expand Up @@ -220,12 +226,21 @@ class ReactantRouter extends BaseReactantRouter {
}

watchRehydratedRouting() {
const stopWatching = watch(
// The first rendering and the hydration of the persistent router may emit actions in a different order due to the module order.
let firstTrigger = false;
const stopWatchingRehydrated = watch(
this,
() => (this as any)[stateKey]._persist?.rehydrated,
(rehydrated) => {
if (!this.enableCacheRouting) {
stopWatchingRehydrated();
}
if (rehydrated) {
stopWatching();
stopWatchingRehydrated();
if (!firstTrigger) {
firstTrigger = true;
return;
}
const router = this._routers[this.portDetector.name];
this._changeRoutingOnSever(
this.portDetector.name,
Expand All @@ -235,7 +250,37 @@ class ReactantRouter extends BaseReactantRouter {
}
}
);
return stopWatching;
const stopWatchingIsFirstRendering = watch(
this,
() => this.portDetector.lastAction.action,
() => {
if (!this.enableCacheRouting) {
stopWatchingIsFirstRendering();
}
const action = this.portDetector.lastAction
.action as any as LocationChangeAction;
if (
action.type === LOCATION_CHANGE &&
action.payload.isFirstRendering
) {
stopWatchingIsFirstRendering();
if (!firstTrigger) {
firstTrigger = true;
return;
}
const router = this._routers[this.portDetector.name];
this._changeRoutingOnSever(
this.portDetector.name,
router ?? this.defaultHistory,
Date.now()
);
}
}
);
return () => {
stopWatchingRehydrated();
stopWatchingIsFirstRendering();
};
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/reactant-share/test/coworker.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,6 @@ describe('base', () => {
expect(onServerFn.mock.calls.length).toBe(0);
expect(subscribeOnServerFn.mock.calls.length).toBe(0);
expect(coworkerModuleSubscribe.mock.calls.map((item) => item[0])).toEqual([
'Coworker',
'counterCoworker',
'Coworker',
'counterCoworker',
Expand All @@ -875,10 +874,11 @@ describe('base', () => {
'counterCoworker',
'Coworker',
'counterCoworker',
'counterCoworker',
'Coworker',
'counterCoworker',
'counterCoworker',
'Coworker',
'counterCoworker',
]);
expect(nonCoworkerModuleSubscribe.mock.calls).toEqual(
Array(10).fill([undefined])
Expand Down
8 changes: 4 additions & 4 deletions packages/reactant-share/test/router.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1809,22 +1809,22 @@ describe('base with storage and router', () => {
expect(onClientFn.mock.calls.length).toBe(1);
expect(subscribeOnClientFn.mock.calls.length).toBe(1);
expect(onServerFn.mock.calls.length).toBe(1);
expect(subscribeOnServerFn.mock.calls.length).toBe(4);
expect(subscribeOnServerFn.mock.calls.length).toBe(2);

await clientApp.bootstrap(clientContainer);

expect(onClientFn.mock.calls.length).toBe(1);
expect(subscribeOnClientFn.mock.calls.length).toBe(2);
expect(onServerFn.mock.calls.length).toBe(1);
expect(subscribeOnServerFn.mock.calls.length).toBe(4);
expect(subscribeOnServerFn.mock.calls.length).toBe(2);
expect(clientContainer.querySelector('#content')?.textContent).toBe('home');

await Promise.resolve();

expect(onClientFn.mock.calls.length).toBe(1);
expect(subscribeOnClientFn.mock.calls.length).toBe(4);
expect(onServerFn.mock.calls.length).toBe(1);
expect(subscribeOnServerFn.mock.calls.length).toBe(6);
expect(subscribeOnServerFn.mock.calls.length).toBe(4);
expect(clientContainer.querySelector('#content')?.textContent).toBe('-0+');

act(() => {
Expand All @@ -1838,7 +1838,7 @@ describe('base with storage and router', () => {
expect(onClientFn.mock.calls.length).toBe(1);
expect(subscribeOnClientFn.mock.calls.length).toBe(6);
expect(onServerFn.mock.calls.length).toBe(1);
expect(subscribeOnServerFn.mock.calls.length).toBe(8);
expect(subscribeOnServerFn.mock.calls.length).toBe(6);
expect(serverApp.instance.router.currentPath).toBe('/counter');
expect(clientApp.instance.router.currentPath).toBe('/counter');
expect(clientContainer.querySelector('#content')?.textContent).toBe('-0+');
Expand Down
Loading