#1480 Output key validation #1628

BALEHOK · 2021-10-11T16:58:42Z

Related to #1480

src/devTools/editor/hooks/usePipelineField.ts

src/devTools/editor/validators/traceErrorGeneralValidator.ts

twschiller · 2021-10-12T12:28:58Z

src/devTools/editor/hooks/usePipelineField.ts


-const REQUIRED_FIELD_REGEX = /^Instance does not have required property "(?<property>.+)"\.$/;
+export type PipelineErrors = string | Record<string, unknown>;


Could you add a documentation comment of what PipelineErrors is supposed to hold?

What are the keys in the record? Are they Formik field paths?

When would it be a string vs. an object?

How to represent if there are no errors? Is it an empty object? Using null?

NIT: We have an UnknownObject alias defined you can use for Record<string, unknown>

Errors are confusing, I agree. Here PipelineErrors is Formik error... thing. It can be a string, a record of strings, or a record of records... I.e. it is dynamic and depends on the level of the state tree where the error happens. Addressing a comment regarding pipelineErrors[0], the error state normally is not an array but since the pipeline is an array we use numbers (index) to get the error related to a block.

Despite it looks like an array (the top-level may look like an array - have numbers for property names), it is an object. I believe we should not type it as an array, that would be even more confusing, for instance, it doesn't have a `length property.

Open for discussion and suggestions here.
Basically we need a typing for the Formik errors.

IMO this is a reasonable explanation - but let's put the explanation in the code instead of the PR so that people who are using the type understand how it's used and why it is the way that it. @BLoe thoughts?

Definitely agree we could add a code comment explaining how this works. It took me a while to understand at first for sure. I think formik is just designed to be super dynamic in the structure here and it doesn't map very well to typescript, or they haven't put a ton of effort yet into getting the typing right for the error object structure.

Btw, the way this works is what makes it possible to use our joinName() logic to construct the field paths to the formik errors, otherwise blockPipeline.2.config.<whatever> wouldn't work properly when we run this:

propertyNameInPipeline = joinName( String(blockIndex), "config", property );

Actually, it appears that the lodash set() function works fine with regular arrays and field paths that include the .<index>.<field> syntax, so that's just lodash magic and doesn't have to do with the error object structure 🤷

src/devTools/editor/tabs/editTab/EditTab.tsx

src/devTools/editor/validators/outputKeyValidator.test.ts

twschiller · 2021-10-12T12:41:39Z

src/devTools/editor/hooks/usePipelineField.ts

-      if (!traceError) {
-        return;
-      }
+  const errorTraceEntry = useSelector(selectTraceError);



Clarifying my performance idea from the outdated pull request:

You can add a useMemo here to convert allBlocks into a Map for faster lookups in the validation. The blockMap variable can then be referenced in the useCallback

const blockMap = useMemo(() => new Map(allBlocks.map(x => [x.id, x]), [allBlocks])

twschiller · 2021-10-12T12:44:48Z

src/devTools/editor/validators/outputKeyValidator.ts

+  allBlocks: IBlock[]
+) {
+  return Promise.all(
+    pipeline


See comment about passing the allBlocks around as a Map instead of as an array

You could also just pre-compute the types of all the blocks as well and pass the types around with the block definitions. I think that would eliminate all the memoizeGetPipelineBlockTypes logic

memoizeGetPipelineBlockTypes did look ugly to me too.

twschiller · 2021-10-12T12:46:50Z

src/devTools/editor/validators/outputKeyValidator.ts

+
+  for (let blockIndex = 0; blockIndex !== pipeline.length; ++blockIndex) {
+    let errorMessage: string;
+    const pipelineBlock = pipeline[blockIndex];


If you wanted to you could use lodashs's zip here instead of manually maintaining the index logic

I could. But I need to know the index, hence for-of doesn't nicely work here. What's left is the regular for. Then it doesn't make much sense to me to zip and run the for loop.

I think the point is that you could do zip(pipeline, blockTypes).forEach((block, type, index) => ...

Definitely mostly a style thing though, so I don't have strong feelings either way.

I love forEach, not allowed on the project though, so only 2 options: for-of or for.

Ah, yeah, that gets a log uglier when you have to use for (const <something> of zip()) { ... }`

and remember about the index ;)

Yeah I mean you can probably do:

for (const [index, value] of zip(...)) { const { block, type } = value; ... }

But, you aren't allowed to move the destructure inside the for loop definition, so at this point it's harder to read than a basic for loop.

twschiller · 2021-10-12T12:48:41Z

I'm OK to get this merged in, but I have a pretty strong preference of avoiding cache logic by just doing the work for the block definitions once:

Put them in a Map from id -> block
Enrich them with the block type information

That would allow constant time (actually linear in the length of the pipeline) validation without the complexity of managing cache invalidation

BLoe

I'm generally in favor of the approach here. I think the logic around allBlocks can definitely be simplified a bit (see my comment about possibly using resolvedBlocks instead). I'm a little concerned with naming conventions with some of this stuff. I don't like the fact that something called a validator is not actually performing any validation, but instead translating/mapping errors from one system to another (trace --> formik).

Otherwise, I think the organization of stuff here makes a lot of sense 👍

BLoe · 2021-10-12T13:26:40Z

src/devTools/editor/hooks/usePipelineField.ts

-  const formikField = useField<BlockPipeline>({
-    name: "extension.blockPipeline",
+  const [{ value: blockPipeline }, { error: blockPipelineErrors }] = useField<
+    BlockConfig[]


You can use the BlockPipeline type alias for BlockConfig[] if you want to

BLoe · 2021-10-12T13:48:26Z

src/devTools/editor/validators/traceErrorInputValidator.ts

+ * Gets Input validation error from the Trace
+ * @param pipelineErrors Pipeline validation errors for the Formik context.
+ * @param traceError Serialized error from running the block ({@link TraceError.error}).
+ * @param blockInstanceId Index of block that generated the Trace Error.


typo blockInstanceId --> blockIndex

Missed it after merge)

BLoe · 2021-10-12T13:55:59Z

src/devTools/editor/validators/traceErrorGeneralValidator.ts

+
+import { TraceError } from "@/telemetry/trace";
+
+function traceErrorGeneralValidator(


Not that big of an issue, but why is this called "General Validator" when it's only mapping trace errors to pipeline error indexes? It doesn't seem like it does any validating itself. To me, a "validator" is something that is running the validation logic, directly. Should we call this and the input one applyTraceInputErrors() and applyTraceGeneralErrors() or mapTraceGeneralErrors() or something like that?

Good point!

BLoe · 2021-10-12T14:25:09Z

src/devTools/editor/validators/outputKeyValidator.ts

+      .map(({ id }) => allBlocks.find((block) => block.id === id))
+      .map(async (block) => (block ? getType(block) : null))


This is the same lookup/map logic used to define resolvedBlocks in EditTab. Should we pass around resolvedBlocks into the validator logic instead of allBlocks? resolvedBlocks is also an array with matching indexes to blockPipeline so it might be easier to use rather than allBlocks.

Not really. resolvedBlocks depends on pipeline, pipeline depends on validation, validation depends on resolvedBlocks. Didn't look nice.

Ah hmm okay, I see what you're saying.

Here's an idea: BlockRegistry is already basically a cache system for block lookups. What if you just searched the registry directly with each block id using the BlockRegistry.lookup() function? Shouldn't this work just as well as a local cache somewhere in the UI component tree?

The only other usage of allBlocks in EditTab is for the add-brick modal. I think that code could be cleaned up in the future too. It could probably be moved down into a child component at some point later.

BLoe · 2021-10-12T14:28:20Z

src/devTools/editor/validators/outputKeyValidator.ts

+
+  for (let blockIndex = 0; blockIndex !== pipeline.length; ++blockIndex) {
+    let errorMessage: string;
+    const pipelineBlock = pipeline[blockIndex];


I think the point is that you could do zip(pipeline, blockTypes).forEach((block, type, index) => ...

Definitely mostly a style thing though, so I don't have strong feelings either way.

BLoe · 2021-10-12T19:19:21Z

src/devTools/editor/hooks/usePipelineField.ts

+      validateOutputKey(formikErrors, pipeline, allBlocks);
+      applyTraceError(formikErrors, errorTraceEntry, pipeline);


Nice, I like these names a lot more 👍

BLoe

I think there are still more improvements to the code to make in the future here, but I'm cool with getting this merged in for now as it is 👍

BALEHOK commented Oct 11, 2021

View reviewed changes

src/devTools/editor/hooks/usePipelineField.ts Outdated Show resolved Hide resolved

BALEHOK requested a review from BLoe October 11, 2021 17:07