[WIP] Initial Pass at Runtime Flag Architecture #579
Conversation
| } | ||
|
|
||
| public getFlag<T>(flag: Flag): T { | ||
| const configuredFlag = dlv(this.flags, flag) as PlayerFlags[Flag]; |
There was a problem hiding this comment.
Note: Technically this doesn't need to use dlv but will clean up when a flag format is agreed on.
| 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; | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
We have the LocalDataStore and the binding format we could use for nesting. Then do something like:
logs.level.duplicateIdThere was a problem hiding this comment.
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.
| private logCacheIssue(id: string, type: NodeType): void { | ||
| let message: string | undefined; | ||
| let loglevel: Severity | undefined; | ||
|
|
||
| if (type === NodeType.Asset || type === NodeType.View) { | ||
| loglevel = this.options.flagController.getFlag("duplicateIDLogLevel"); | ||
| message = `Cache conflict: Found Asset/View nodes that have conflicting ids: ${id}, may cause cache issues.`; | ||
| } else if (type === NodeType.Value) { | ||
| loglevel = this.options.flagController.getFlag("cacheConflictLogLevel"); | ||
| message = `Cache conflict: Found Value nodes that have conflicting ids: ${id}, may cause cache issues. To improve performance make value node IDs globally unique.`; | ||
| } | ||
|
|
||
| if (message && loglevel) { | ||
| this.logger?.[loglevel]?.(message); | ||
| } | ||
| } |
There was a problem hiding this comment.
While most usages of flags I can think of right now are around logging but in theory some runtime functionality could depend on flags (like errors during expression parsing). Based on the current model a reference to the FlagController would have to be passed around but we could expose some dynamic logging functionality in the logger to take a flag and a message and let it internally figure out the severity to log with which would remove the need to pass around the reference as much.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #579 +/- ##
==========================================
- Coverage 89.76% 85.85% -3.91%
==========================================
Files 331 189 -142
Lines 19840 10718 -9122
Branches 1949 1950 +1
==========================================
- Hits 17809 9202 -8607
+ Misses 2017 1502 -515
Partials 14 14 ☔ View full report in Codecov by Sentry. |
Background
Player strives to be as configurable as possible and primarily achieves this through it's plugin architecture. While this does allow almost any aspect of Player's behavior to be configured, there are still some parts of Player's internal working that users of Player don't have access to. These are things like how strict Player is while running and how it should recover from state errors. For the most part the Player team has made assumptions on how users of Player would want Player to work but in pursuit of fulfilling Player's goal of customizability and configurability we should allow this behavior to be customizable.
Implementation Details
[WIP]
Change Type (required)
Indicate the type of change your pull request is:
patchminormajorN/ADoes your PR have any documentation updates?
Release Notes
Add the ability to statically/dynamically configure Player's runtime behavior via "flags". Flags are held in a
FlagControllerclass that can either get flag configuration via Player's initial configuration, dynamically at runtime via a function to set new flags, or even more dynamically by tapping a hook on the controller to override the value of any specific flag(s).