Proposal: Batch Update For Trait
MrWindlike opened this issue · 1 comments
Currently, the same trait would be executed multiple times in a React update cycle. If there are side effects in the trait, some unexpected results may occur. So, this Issue discusses how to solve this problem.
Phenomenon
Let’s take a look the example of this problem. Here are a log trait, it just log the params value when they change.
import { Type } from '@sinclair/typebox';
import { implementRuntimeTrait } from '../../utils/buildKit';
import { CORE_VERSION } from '@sunmao-ui/shared';
export const LogTraitPropertiesSpec = Type.Object({
param1: Type.Any({
title: 'Param 1',
}),
param2: Type.Any({
title: 'Param 2',
}),
});
export default implementRuntimeTrait({
version: CORE_VERSION,
metadata: {
name: 'Log',
description: '',
isDataSource: true,
},
spec: {
properties: LogTraitPropertiesSpec,
state: Type.Any(),
methods: [],
},
})(() => {
return ({ param1, param2 }) => {
console.log(`log: param1(${param1}), param2(${param2})`);
return {
props: {},
};
};
});We pass the {{state.value}} and the {{!state.value}} as the values of the param1 and the param2 . Here is the full App schema:
{
"kind":"Application",
"version":"example/v1",
"metadata":{
"name":"sunmao application",
"description":"sunmao empty application"
},
"spec":{
"components":[
{
"id":"state",
"type":"core/v1/dummy",
"properties":{
},
"traits":[
{
"type":"core/v1/state",
"properties":{
"key":"value",
"initialValue":"value"
}
}
]
},
{
"id":"button",
"type":"chakra_ui/v1/button",
"properties":{
"text":{
"raw":"Button",
"format":"plain"
},
"isLoading":false,
"colorScheme":"blue"
},
"traits":[
{
"type":"core/v1/event",
"properties":{
"handlers":[
{
"type":"onClick",
"componentId":"state",
"method":{
"name":"setValue",
"parameters":{
"key":"value",
"value":""
}
},
"wait":{
"type":"debounce",
"time":0
},
"disabled":false
}
]
}
},
{
"type":"core/v1/Log",
"properties":{
"param1":"{{state.value}}",
"param2":"{{!state.value}}"
}
}
]
}
]
}
}After that we click the button to change the state ‘s value once, and then check the logs:
Log.tsx:28 log: param1(), param2(false)
Log.tsx:28 log: param1(), param2(true)As we can see, it logs twice when the dependent state only changes once. If traits implement the logic of side effects, they can cause unexpected errors, which should not be expected.
Solutions
Batch Execute For The Trait
We should probably wait until all of the Trait's properties have been updated in one React update cycle before executing the Trait. This avoids the problem of executing the Trait multiple times in one React update cycle.
// ImplWrapperMain.tsx
const [traitPropertiesMap, setTraitPropertiesMap] = useState({});
// batch update the properties of traits
useEffect(() => {
const stops: ReturnType<typeof watch>[] = [];
const properties: Array<RuntimeTraitSchema['properties']> = [];
c.traits.forEach((t, i) => {
const { result, stop } = stateManager.deepEvalAndWatch(
t.properties,
({ result: property }: any) => {
setTraitPropertiesMap({
set(traitPropertiesMap, t.type, property);
})
},
{
slotKey,
fallbackWhenError: () => undefined,
}
);
stops.push(stop);
properties.push(result);
});
setTraitPropertiesMap(c.traits.reduce((result, trait, i)=> {
result[trait.type] = properties[i];
return result;
}, {}));
return () => stops.forEach(s => s());
}, [...]);
// execute traits after the properties changed
useEffect(()=> {
c.traits.forEach((t, i) => {
const traitResult = executeTrait(t, property);
setTraitResults(oldResults => {
// assume traits number and order will not change
const newResults = [...oldResults];
newResults[i] = traitResult;
return newResults;
});
});
}, [...]);But the problem with this solution is that it doesn't execute the Trait until the next iteration of React, which is a potentially risky change from the original mechanism.
Batch Execute For Side Effects
Maybe we don't need to do a batch update on the whole Trait, but only on the side effects. For example, we provide a batch execution method batchExecute for traits to perform side effects.
// trait
const { batchExecute } = props.services;
function sideEffect() {
...
}
batchExecute(sideEffect);// ImplWrapperMain.tsx
const [batchExecuteMap, setBatchExecuteMap] = useState({});
const batchExecute = useCallback((trait, fn) {
setBatchExecuteQueue((map)=> {
if (map[trait.type]) return map;
const newBatchExecuteMap = set(map, trait.type, fn);
return newBatchExecuteMap;
});
}, [...]);
useEffect(()=> {
batchExecuteMap.forEach(fn=> fn());
setBatchExecuteMap({});
}, [batchExecuteMap]);Add traitPropertiesDidUpdated
Or we can provide a traitPropertiesDidUpdated lifecycle function to traits to perform side effects.
// ImplWrapperMain.tsx
const [traitPropertiesDidUpdatedHandlers, setTraitPropertiesDidUpdatedHandlers] = useState([]);
// eval traits' properties then execute traits
useEffect(() => {
const stops: ReturnType<typeof watch>[] = [];
const properties: Array<RuntimeTraitSchema['properties']> = [];
c.traits.forEach((t, i) => {
const { result, stop } = stateManager.deepEvalAndWatch(
t.properties,
({ result: property }: any) => {
const traitResult = executeTrait(t, property);
...
if (traitResult.traitPropertiesDidUpdated) {
setTraitPropertiesDidUpdatedHandlers((handlers)=> {
handlers = handlers.concat(traitResult.traitPropertiesDidUpdated);
return handlers;
})
}
},
{
slotKey,
fallbackWhenError: () => undefined,
}
);
stops.push(stop);
properties.push(result);
});
...
}, [...]);
useEffect(() => {
const clearFunctions = traitPropertiesDidUpdatedHandlers.map(fn => fn());
setTraitPropertiesDidUpdatedHandlers([]);
return () => {
clearFunctions?.forEach(func => func && func());
};
}, [traitPropertiesDidUpdatedHandlers]);Don’t Change The Code In Runtime
The final solution is that we don't deal with this issue at runtime, but instead ask users to write traits without executing logic that has side effects. If the users need to perform side effects, they can write components to do so.
Conclusion
I prefer the third option, which is to provide a new lifecycle function traitPropertiesDidUpdated to traits.
The reasons are as follows:
The first solution solves the problem, but it changes the original implementation mechanism and has the possibility of causing the original project to go wrong unexpectedly.
The second option does not break the original mechanism, but providing a single function to handle side effects may increase the user's understanding cost.
The third, more intuitive solution is to provide a new lifecycle function that executes when a Trait property changes, allowing users to do anything in it, including side effects that they don't want to repeat.
The last one requires users to modify their own code and is unfriendly because it doesn't address the limitations of traits.
I prefer the first way although it is the hardest way. This problem not only occurs in trait, but also in component. If a component has multiple expressions which use the same state, it also will render multiple times.
The root cause is that we watch the changes in expression level, not in component property level. If we fix it as so, it will be a big change, which need more attention to see the influence.
deepEvalAndWatch<T extends Record<string, unknown> | any[] | string>(
value: T,
watcher: (params: { result: EvaledResult<T> }) => void,
options: EvalOptions = {}
) {
const stops: ReturnType<typeof watch>[] = [];
// just eval
const evaluated = this.deepEval(value, options) as T extends string
? unknown
: PropsAfterEvaled<Exclude<T, string>>;
const store = this.slotStore;
options.scopeObject = {
...options.scopeObject,
$slot: options.slotKey ? store[options.slotKey] : undefined,
};
// watch change
if (value && typeof value === 'object') {
let resultCache = evaluated as PropsAfterEvaled<Exclude<T, string>>;
this.mapValuesDeep(value, ({ value, path }) => {
const isDynamicExpression =
typeof value === 'string' &&
parseExpression(value).some(exp => typeof exp !== 'string');
if (!isDynamicExpression) return;
const stop = watch(
() => {
const result = this._maskedEval(value as string, options);
return result;
},
newV => {
resultCache = produce(resultCache, draft => {
set(draft, path, newV);
});
watcher({ result: resultCache as EvaledResult<T> });
}
);
stops.push(stop);
});
The third way will not break current code, but it seems too temporary. I think we'd better check how big influence the first way will bring. If it is not as big as we think, we should go with it. If it is really very big, then we can go the third way.