#3831 Add validation support for sub pipelines #3832
Conversation
BALEHOK
force-pushed
the
feature/3831-subpipelines-validation
branch
from
6148991
to
9d3f648
Compare
| * Join parts of a path, ignoring null/blank parts. | ||
| * @param nameParts the parts of the name | ||
| */ | ||
| export function joinPathParts(...nameParts: Array<string | number>): string { |
There was a problem hiding this comment.
Could we add some documentation on why/when the developer should use this method vs. joinName above?
| @@ -49,6 +49,7 @@ const specialCharsRegex = /[.[\]]/; | |||
|
|
|||
| /** | |||
| * Create a Formik field name, validating the individual path parts. | |||
| * Wraps parts with special characters in brackets, so Formik treat it as a single property name. | |||
|
@BALEHOK thanks for the PR! I checked it out and it's working for me. I'll provide some minor wording tweaks on the error messages. @BLoe please review for architecture/details One thing that could be confusing is the behavior for nested bricks. In the parent brick, if there's no errors in the brick itself, I think we need a message that says: "Error in one or more nested bricks. Click on the brick in the Extension Overview to view the error" to clarify that the parent brick itself doesn't have any errors
|
| import { joinName } from "@/utils"; | ||
| import { joinName, joinPathParts } from "@/utils"; | ||
| import { traversePipeline } from "@/pageEditor/utils"; | ||
| import { setPipelineBlockError } from "./setPipelineBlockError"; | ||
|
|
||
| const MUSTACHE_ERROR_MESSAGE = | ||
| "Invalid string template. Read more about string templates: https://docs.pixiebrix.com/nunjucks-templates"; |
There was a problem hiding this comment.
Let's change the name of this constant to TEMPLATE_ERROR_MESSAGE
Let's change to "Invalid text template. Read more about text templates: https://docs.pixiebrix.com/nunjucks-templates"
| @@ -50,23 +50,15 @@ const forEachBlock = new ForEach(); | |||
| const immediateUserEvent = userEvent.setup({ delay: null }); | |||
|
|
|||
| beforeAll(async () => { | |||
| jest.useFakeTimers(); | |||
|
|
|||
| jest.setTimeout(30_000); // This test is flaky with the default timeout of 5000 ms | |||
There was a problem hiding this comment.
Does this work for you? I think I tried this before and it didn't work. Doesn't it have to be top-level in the file? Also it just applies to the current file so I don't think you have to "clean it up" and set it back to the previous value? I may be wrong here but this is exactly what I tried to do at first and it wasn't working.
There was a problem hiding this comment.
It works for me, yes. Maybe it was failing for you because of jest cache. You can clear it with jest flag:
--clearCache. With npm it is: npm run test -- --clearCache
There was a problem hiding this comment.
As long as this works locally and on CI, then I'm fine with it. Just surprising since it wasn't working when I tried it like that.
There was a problem hiding this comment.
Never mind, I've reverted this change
| <span | ||
| style="color: rgb(241, 89, 80);" | ||
| > | ||
| undefined |
There was a problem hiding this comment.
Is it supposed to print undefined in the UI like this?
There was a problem hiding this comment.
yep, in the test. this is the @input of the extension reported to the Data Panel. normally it is the HTML document meta data, but in the test there's no document and we get undefined
What does this PR do?
Demo
Nunjucks validation:
Renderers validation:
Inputs validation:
Trace errors:
Future Work
Checklist