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

[WIP] Initial Pass at Runtime Flag Architecture #579

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
32 changes: 32 additions & 0 deletions core/player/src/controllers/flags/__tests__/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { describe, it, expect } from "vitest";
import { FlagController } from "../controller";
import { PlayerFlags } from "../types";

describe("Controller Functionality", () => {
const mockFlags: PlayerFlags = {
duplicateIDLogLevel: "error",
};

it("Basic Functionality", () => {
const controller = new FlagController(mockFlags);

expect(controller.getFlag("duplicateIDLogLevel")).toBe("error");

controller.updateFlags({ duplicateIDLogLevel: "debug" });

expect(controller.getFlag("duplicateIDLogLevel")).toBe("debug");
});

it("Hooks", () => {
const controller = new FlagController(mockFlags);

controller.hooks.overrideFlag.tap("test", (value, key) => {
expect(value).toBe("error");
expect(key).toBe("duplicateIDLogLevel");

return "warning";
});

expect(controller.getFlag("duplicateIDLogLevel")).toBe("warning");
});
});
32 changes: 32 additions & 0 deletions core/player/src/controllers/flags/controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import dlv from "dlv";

import { PlayerFlags, Flag, DefaultFlags } from "./types";
import { SyncWaterfallHook } from "tapable-ts";

export class FlagController {
private flags: PlayerFlags;

constructor(flags?: PlayerFlags) {
this.flags = { ...DefaultFlags, ...flags };
}

/** Hooks for the FlagsController */
public readonly hooks: {
/** Allow a plugin or integration to dynamically change a flag without setting it globally */
overrideFlag: SyncWaterfallHook<[any, string]>;
} = {
overrideFlag: new SyncWaterfallHook(),
};

public updateFlags(newFlags: Partial<PlayerFlags>): void {
this.flags = {
...this.flags,
...newFlags,
};
}

public getFlag<T>(flag: Flag): T {
const configuredFlag = dlv(this.flags, flag) as PlayerFlags[Flag];
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: Technically this doesn't need to use dlv but will clean up when a flag format is agreed on.

return this.hooks.overrideFlag.call(configuredFlag, flag);
}
}
2 changes: 2 additions & 0 deletions core/player/src/controllers/flags/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from "./controller";
export * from "./types";
17 changes: 17 additions & 0 deletions core/player/src/controllers/flags/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { Severity } from "../../logger";

/** Configuration for runtime Player behavior */
export interface PlayerFlags {
/** What log level duplicate ID errors during view resolution should be raised as */
duplicateIDLogLevel: Severity;

/** Log level for when there are cache conflicts in view resolution (usually related to duplicate IDs) */
cacheConflictLogLevel: Severity;
}
Comment on lines +4 to +10
Copy link
Member Author

Choose a reason for hiding this comment

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

Open to feedback on the flag format. For simplicity this initial pass was built to use flat flags primarily because nested flags would take more logic to merge and operate on and pulling in another dependency would be preferable to writing that logic custom but something like that would add a few kb to Player's size

Copy link
Member

Choose a reason for hiding this comment

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

We have the LocalDataStore and the binding format we could use for nesting. Then do something like:

logs.level.duplicateId

Copy link
Member Author

Choose a reason for hiding this comment

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

I was considering using that but it seemed like overkill complexity-wise and potentially adding some overhead to get a flag if we need to go through binding parsing (even if its cached). I can write some benchmarks for it though to see what the impact would be.


export const DefaultFlags: PlayerFlags = {
duplicateIDLogLevel: "error",
cacheConflictLogLevel: "info",
};

export type Flag = keyof PlayerFlags;
1 change: 1 addition & 0 deletions core/player/src/controllers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ export * from "./validation";
export * from "./view";
export * from "./data/controller";
export * from "./constants";
export * from "./flags";
16 changes: 14 additions & 2 deletions core/player/src/controllers/view/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type { DataController } from "../data/controller";
import { AssetTransformCorePlugin } from "./asset-transform";
import type { TransformRegistry } from "./types";
import type { BindingInstance } from "../../binding";
import { FlagController } from "../flags";

export interface ViewControllerOptions {
/** Where to get data from */
Expand All @@ -22,11 +23,22 @@ export interface ViewControllerOptions {

/** A flow-controller instance to listen for view changes */
flowController: FlowController;

/** A FlagController instance to */
flagController: FlagController;
}

/** A controller to manage updating/switching views */
export class ViewController {
public readonly hooks = {
public readonly hooks: {
/** Do any processing before the `View` instance is created */
resolveView: SyncWaterfallHook<
[View | undefined, string, NavigationFlowViewState],
Record<string, any>
>;
// The hook right before the View starts resolving. Attach anything custom here
view: SyncHook<[ViewInstance], Record<string, any>>;
} = {
/** Do any processing before the `View` instance is created */
resolveView: new SyncWaterfallHook<
[View | undefined, string, NavigationFlowViewState]
Expand Down Expand Up @@ -156,7 +168,7 @@ export class ViewController {
}
}

public onView(state: NavigationFlowViewState) {
public onView(state: NavigationFlowViewState): void {
const viewId = state.ref;

const source = this.hooks.resolveView.call(
Expand Down
45 changes: 40 additions & 5 deletions core/player/src/player.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,14 @@ import { SchemaController } from "./schema";
import { BindingParser } from "./binding";
import type { ViewInstance } from "./view";
import { resolveDataRefs } from "./string-resolver";
import type { FlowInstance } from "./controllers";
import type { FlowInstance, PlayerFlags } from "./controllers";
import {
ConstantsController,
ViewController,
DataController,
ValidationController,
FlowController,
FlagController,
} from "./controllers";
import { FlowExpPlugin } from "./plugins/flow-exp-plugin";
import { DefaultExpPlugin } from "./plugins/default-exp-plugin";
Expand Down Expand Up @@ -68,6 +69,9 @@ export interface PlayerConfigOptions {

/** A logger to use */
logger?: Logger;

/** Flags to configure runtime Player behavior */
flags?: PlayerFlags;
}

export interface PlayerInfo {
Expand All @@ -87,12 +91,39 @@ export class Player {
commit: COMMIT,
};

public readonly logger = new TapableLogger();
public readonly constantsController = new ConstantsController();
public readonly logger: TapableLogger = new TapableLogger();
public readonly constantsController: ConstantsController =
new ConstantsController();
public readonly flagsController: FlagController;
private config: PlayerConfigOptions;
private state: PlayerFlowState = NOT_STARTED_STATE;

public readonly hooks = {
public readonly hooks: {
/** The hook that fires every time we create a new flowController (a new Content blob is passed in) */
flowController: SyncHook<[FlowController], Record<string, any>>;
/** The hook that updates/handles views */
viewController: SyncHook<[ViewController], Record<string, any>>;
/** A hook called every-time there's a new view. This is equivalent to the view hook on the view-controller */
view: SyncHook<[ViewInstance], Record<string, any>>;
/** Called when an expression evaluator was created */
expressionEvaluator: SyncHook<[ExpressionEvaluator], Record<string, any>>;
/** The hook that creates and manages data */
dataController: SyncHook<[DataController], Record<string, any>>;
/** Called after the schema is created for a flow */
schema: SyncHook<[SchemaController], Record<string, any>>;
/** Manages validations (schema and x-field ) */
validationController: SyncHook<[ValidationController], Record<string, any>>;
/** Manages parsing binding */
bindingParser: SyncHook<[BindingParser], Record<string, any>>;
/** A that's called for state changes in the flow execution */
state: SyncHook<[PlayerFlowState], Record<string, any>>;
/** A hook to access the current flow */
onStart: SyncHook<[FlowType], Record<string, any>>;
/** A hook for when the flow ends either in success or failure */
onEnd: SyncHook<[], Record<string, any>>;
/** Mutate the Content flow before starting */
resolveFlowContent: SyncWaterfallHook<[FlowType], Record<string, any>>;
} = {
/** The hook that fires every time we create a new flowController (a new Content blob is passed in) */
flowController: new SyncHook<[FlowController]>(),

Expand Down Expand Up @@ -125,6 +156,7 @@ export class Player {

/** A hook for when the flow ends either in success or failure */
onEnd: new SyncHook<[]>(),

/** Mutate the Content flow before starting */
resolveFlowContent: new SyncWaterfallHook<[FlowType]>(),
};
Expand All @@ -134,6 +166,8 @@ export class Player {
this.logger.addHandler(config.logger);
}

this.flagsController = new FlagController(config?.flags);

this.config = config || {};
this.config.plugins = [
new DefaultExpPlugin(),
Expand Down Expand Up @@ -171,7 +205,7 @@ export class Player {
}

/** Register and apply [Plugin] if one with the same symbol is not already registered. */
public registerPlugin(plugin: PlayerPlugin) {
public registerPlugin(plugin: PlayerPlugin): void {
plugin.apply(this);
this.config.plugins?.push(plugin);
}
Expand Down Expand Up @@ -419,6 +453,7 @@ export class Player {
type: (b) => schema.getType(parseBinding(b)),
},
constants: this.constantsController,
flagController: this.flagsController,
});
viewController.hooks.view.tap("player", (view) => {
validationController.onView(view);
Expand Down
17 changes: 13 additions & 4 deletions core/player/src/view/resolver/__tests__/edgecases.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { describe, it, expect, vitest } from "vitest";
import { describe, it, expect, vitest, MockedFunction } from "vitest";
import { replaceAt, set, omit } from "timm";
import { BindingParser } from "../../../binding";
import { ExpressionEvaluator } from "../../../expressions";
import { LocalModel, withParser } from "../../../data";
import { SchemaController } from "../../../schema";
import type { Logger } from "../../../logger";
import type { Logger, LogFn } from "../../../logger";
import { TapableLogger } from "../../../logger";
import { Resolver } from "..";
import type { Node } from "../../parser";
Expand All @@ -14,6 +14,7 @@ import {
MultiNodePlugin,
AssetPlugin,
} from "../../plugins";
import { FlagController } from "../../../controllers";

describe("Dynamic AST Transforms", () => {
const content = {
Expand Down Expand Up @@ -58,6 +59,7 @@ describe("Dynamic AST Transforms", () => {
model: withParser(model, bindingParser.parse),
}),
schema: new SchemaController(),
flagController: new FlagController(),
});

// basic transform to change the asset
Expand Down Expand Up @@ -168,6 +170,7 @@ describe("Dynamic AST Transforms", () => {
model: withParser(model, bindingParser.parse),
}),
schema: new SchemaController(),
flagController: new FlagController(),
});

resolver.update();
Expand Down Expand Up @@ -237,6 +240,7 @@ describe("Dynamic AST Transforms", () => {
model: withParser(model, bindingParser.parse),
}),
schema: new SchemaController(),
flagController: new FlagController(),
});

let inputNode: Node.Node | undefined;
Expand Down Expand Up @@ -290,6 +294,7 @@ describe("Dynamic AST Transforms", () => {
model: withParser(model, bindingParser.parse),
}),
schema: new SchemaController(),
flagController: new FlagController(),
});

let parent;
Expand Down Expand Up @@ -387,6 +392,7 @@ describe("Duplicate IDs", () => {
}),
schema: new SchemaController(),
logger,
flagController: new FlagController(),
});

new StringResolverPlugin().applyResolver(resolver);
Expand All @@ -397,7 +403,7 @@ describe("Duplicate IDs", () => {
expect(testLogger.error).toBeCalledWith(
"Cache conflict: Found Asset/View nodes that have conflicting ids: action-1, may cause cache issues.",
);
(testLogger.error as jest.Mock).mockClear();
(testLogger.error as MockedFunction<LogFn>).mockClear();

expect(firstUpdate).toStrictEqual({
id: "action",
Expand Down Expand Up @@ -484,6 +490,7 @@ describe("Duplicate IDs", () => {
}),
schema: new SchemaController(),
logger,
flagController: new FlagController(),
});

new StringResolverPlugin().applyResolver(resolver);
Expand All @@ -494,7 +501,7 @@ describe("Duplicate IDs", () => {
expect(testLogger.info).toBeCalledWith(
"Cache conflict: Found Value nodes that have conflicting ids: value-1, may cause cache issues. To improve performance make value node IDs globally unique.",
);
(testLogger.info as jest.Mock).mockClear();
(testLogger.info as MockedFunction<LogFn>).mockClear();
expect(firstUpdate).toStrictEqual(content);

resolver.update();
Expand Down Expand Up @@ -535,6 +542,7 @@ describe("AST caching", () => {
model: withParser(model, bindingParser.parse),
}),
schema: new SchemaController(),
flagController: new FlagController(),
});

const resolvedNodes: any[] = [];
Expand Down Expand Up @@ -602,6 +610,7 @@ describe("Root AST Immutability", () => {
model: withParser(model, bindingParser.parse),
}),
schema: new SchemaController(),
flagController: new FlagController(),
});
let finalNode;

Expand Down
Loading