Skip to content

Commit

Permalink
[Expressions] Fix setup and start contracts (#110841)
Browse files Browse the repository at this point in the history 
* Refactor executor forking to implement state inheritance
* Fix setup and start contracts typings
* Add support of named forks
* Add expressions service life-cycle assertions
  • Loading branch information
@dokmic
dokmic authored Sep 23, 2021
1 parent 5b73ab1 commit c06d604
Show file tree
Hide file tree
Showing 14 changed files with 320 additions and 231 deletions.
2 changes: 1 addition & 1 deletion src/plugins/expressions/common/execution/execution.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export class Execution<
const ast = execution.ast || parseExpression(this.expression);

this.state = createExecutionContainer({
...executor.state.get(),
...executor.state,
state: 'not-started',
ast,
});
Expand Down
117 changes: 63 additions & 54 deletions src/plugins/expressions/common/executor/executor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export class TypesRegistry implements IRegistry<ExpressionType> {
}

public get(id: string): ExpressionType | null {
return this.executor.state.selectors.getType(id);
return this.executor.getType(id) ?? null;
}

public toJS(): Record<string, ExpressionType> {
Expand All @@ -71,7 +71,7 @@ export class FunctionsRegistry implements IRegistry<ExpressionFunction> {
}

public get(id: string): ExpressionFunction | null {
return this.executor.state.selectors.getFunction(id);
return this.executor.getFunction(id) ?? null;
}

public toJS(): Record<string, ExpressionFunction> {
Expand All @@ -95,22 +95,44 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
return executor;
}

public readonly state: ExecutorContainer<Context>;
public readonly container: ExecutorContainer<Context>;

/**
* @deprecated
*/
public readonly functions: FunctionsRegistry;
public readonly functions = new FunctionsRegistry(this);

/**
* @deprecated
*/
public readonly types: TypesRegistry;
public readonly types = new TypesRegistry(this);

protected parent?: Executor<Context>;

constructor(state?: ExecutorState<Context>) {
this.state = createExecutorContainer<Context>(state);
this.functions = new FunctionsRegistry(this);
this.types = new TypesRegistry(this);
this.container = createExecutorContainer<Context>(state);
}

public get state(): ExecutorState<Context> {
const parent = this.parent?.state;
const state = this.container.get();

return {
...(parent ?? {}),
...state,
types: {
...(parent?.types ?? {}),
...state.types,
},
functions: {
...(parent?.functions ?? {}),
...state.functions,
},
context: {
...(parent?.context ?? {}),
...state.context,
},
};
}

public registerFunction(
Expand All @@ -119,15 +141,18 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
const fn = new ExpressionFunction(
typeof functionDefinition === 'object' ? functionDefinition : functionDefinition()
);
this.state.transitions.addFunction(fn);
this.container.transitions.addFunction(fn);
}

public getFunction(name: string): ExpressionFunction | undefined {
return this.state.get().functions[name];
return this.container.get().functions[name] ?? this.parent?.getFunction(name);
}

public getFunctions(): Record<string, ExpressionFunction> {
return { ...this.state.get().functions };
return {
...(this.parent?.getFunctions() ?? {}),
...this.container.get().functions,
};
}

public registerType(
Expand All @@ -136,23 +161,30 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
const type = new ExpressionType(
typeof typeDefinition === 'object' ? typeDefinition : typeDefinition()
);
this.state.transitions.addType(type);

this.container.transitions.addType(type);
}

public getType(name: string): ExpressionType | undefined {
return this.state.get().types[name];
return this.container.get().types[name] ?? this.parent?.getType(name);
}

public getTypes(): Record<string, ExpressionType> {
return { ...this.state.get().types };
return {
...(this.parent?.getTypes() ?? {}),
...this.container.get().types,
};
}

public extendContext(extraContext: Record<string, unknown>) {
this.state.transitions.extendContext(extraContext);
this.container.transitions.extendContext(extraContext);
}

public get context(): Record<string, unknown> {
return this.state.selectors.getContext();
return {
...(this.parent?.context ?? {}),
...this.container.selectors.getContext(),
};
}

/**
Expand Down Expand Up @@ -199,18 +231,15 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u
) {
for (const link of ast.chain) {
const { function: fnName, arguments: fnArgs } = link;
const fn = getByAlias(this.state.get().functions, fnName);
const fn = getByAlias(this.getFunctions(), fnName);

if (fn) {
// if any of arguments are expressions we should migrate those first
link.arguments = mapValues(fnArgs, (asts, argName) => {
return asts.map((arg) => {
if (arg && typeof arg === 'object') {
return this.walkAst(arg, action);
}
return arg;
});
});
link.arguments = mapValues(fnArgs, (asts) =>
asts.map((arg) =>
arg != null && typeof arg === 'object' ? this.walkAst(arg, action) : arg
)
);

action(fn, link);
}
Expand Down Expand Up @@ -275,39 +304,19 @@ export class Executor<Context extends Record<string, unknown> = Record<string, u

private migrate(ast: SerializableRecord, version: string) {
return this.walkAst(cloneDeep(ast) as ExpressionAstExpression, (fn, link) => {
if (!fn.migrations[version]) return link;
const updatedAst = fn.migrations[version](link) as ExpressionAstFunction;
link.arguments = updatedAst.arguments;
link.type = updatedAst.type;
if (!fn.migrations[version]) {
return;
}

({ arguments: link.arguments, type: link.type } = fn.migrations[version](
link
) as ExpressionAstFunction);
});
}

public fork(): Executor<Context> {
const initialState = this.state.get();
const fork = new Executor<Context>(initialState);

/**
* Synchronize registry state - make any new types, functions and context
* also available in the forked instance of `Executor`.
*/
this.state.state$.subscribe(({ types, functions, context }) => {
const state = fork.state.get();
fork.state.set({
...state,
types: {
...types,
...state.types,
},
functions: {
...functions,
...state.functions,
},
context: {
...context,
...state.context,
},
});
});
const fork = new Executor<Context>();
fork.parent = this;

return fork;
}
Expand Down
52 changes: 45 additions & 7 deletions src/plugins/expressions/common/service/expressions_services.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,16 @@ describe('ExpressionsService', () => {
const expressions = new ExpressionsService();

expect(expressions.setup()).toMatchObject({
getFunction: expect.any(Function),
getFunctions: expect.any(Function),
getRenderer: expect.any(Function),
getRenderers: expect.any(Function),
getType: expect.any(Function),
getTypes: expect.any(Function),
registerFunction: expect.any(Function),
registerType: expect.any(Function),
registerRenderer: expect.any(Function),
run: expect.any(Function),
fork: expect.any(Function),
});
});

Expand All @@ -30,7 +35,16 @@ describe('ExpressionsService', () => {
expressions.setup();

expect(expressions.start()).toMatchObject({
getFunction: expect.any(Function),
getFunctions: expect.any(Function),
getRenderer: expect.any(Function),
getRenderers: expect.any(Function),
getType: expect.any(Function),
getTypes: expect.any(Function),
registerFunction: expect.any(Function),
registerType: expect.any(Function),
registerRenderer: expect.any(Function),
execute: expect.any(Function),
run: expect.any(Function),
});
});
Expand All @@ -54,21 +68,21 @@ describe('ExpressionsService', () => {
const service = new ExpressionsService();
const fork = service.fork();

expect(fork.executor.state.get().types).toEqual(service.executor.state.get().types);
expect(fork.getTypes()).toEqual(service.getTypes());
});

test('fork keeps all functions of the origin service', () => {
const service = new ExpressionsService();
const fork = service.fork();

expect(fork.executor.state.get().functions).toEqual(service.executor.state.get().functions);
expect(fork.getFunctions()).toEqual(service.getFunctions());
});

test('fork keeps context of the origin service', () => {
const service = new ExpressionsService();
const fork = service.fork();

expect(fork.executor.state.get().context).toEqual(service.executor.state.get().context);
expect(fork.executor.state.context).toEqual(service.executor.state.context);
});

test('newly registered functions in origin are also available in fork', () => {
Expand All @@ -82,7 +96,7 @@ describe('ExpressionsService', () => {
fn: () => {},
});

expect(fork.executor.state.get().functions).toEqual(service.executor.state.get().functions);
expect(fork.getFunctions()).toEqual(service.getFunctions());
});

test('newly registered functions in fork are NOT available in origin', () => {
Expand All @@ -96,14 +110,15 @@ describe('ExpressionsService', () => {
fn: () => {},
});

expect(Object.values(fork.executor.state.get().functions)).toHaveLength(
Object.values(service.executor.state.get().functions).length + 1
expect(Object.values(fork.getFunctions())).toHaveLength(
Object.values(service.getFunctions()).length + 1
);
});

test('fork can execute an expression with newly registered function', async () => {
const service = new ExpressionsService();
const fork = service.fork();
fork.start();

service.registerFunction({
name: '__test__',
Expand All @@ -118,5 +133,28 @@ describe('ExpressionsService', () => {

expect(result).toBe('123');
});

test('throw on fork if the service is already started', async () => {
const service = new ExpressionsService();
service.start();

expect(() => service.fork()).toThrow();
});
});

describe('.execute()', () => {
test('throw if the service is not started', () => {
const expressions = new ExpressionsService();

expect(() => expressions.execute('foo', null)).toThrow();
});
});

describe('.run()', () => {
test('throw if the service is not started', () => {
const expressions = new ExpressionsService();

expect(() => expressions.run('foo', null)).toThrow();
});
});
});
Loading

0 comments on commit c06d604

Please sign in to comment.