#7265: Move Nunjucks template validation to sandbox #7426
Conversation
fregante
changed the title
|
@fregante see comment here on how we currently handle async methods in analyses: #7265 (comment) |
|
Tagging @grahamlangford and @fungairino as reviewers for awareness around approach/discussion: #7426 (comment) |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7426 +/- ##
=======================================
Coverage 72.61% 72.61%
=======================================
Files 1211 1211
Lines 37867 37886 +19
Branches 7117 7122 +5
=======================================
+ Hits 27497 27511 +14
- Misses 10370 10375 +5 ☔ View full report in Codecov by Sentry. |
| origins: [parsedURL.href], | ||
| }) | ||
| // eslint-disable-next-line promise/prefer-await-to-then -- need the complete Promise | ||
| .then((hasPermission) => { |
There was a problem hiding this comment.
Nit, before:
// eslint-disable-next-line promise/prefer-await-to-then -- need the complete Promise
const promise = x().then(y => {
z(y);
});After:
const promise = (async () => {
const y = await x();
z(y);
})();While a bit more verbose, it's easier to deal with linear await code than then/catch pipelines, which is the reason behind promise/prefer-await-to-then. Once the eslint-disable-next-line comment is factored in, it's actually shorter.
Another example
pixiebrix-extension/src/starterBricks/tourController.ts
Lines 275 to 284 in 7ad11c7
There was a problem hiding this comment.
Agreed. I wonder what the thought process was behind using the promise callback in the first place.
There was a problem hiding this comment.
I don't know a good reason off the top of my head. In some places we used to use it because microtasks threw off browser user gesture recognition, but that wouldn't apply here
| @@ -41,6 +42,10 @@ type PushAnnotationArgs = { | |||
| }; | |||
|
|
|||
| class TemplateAnalysis extends PipelineExpressionVisitor implements Analysis { | |||
| // XXX: for now we handle asynchronous pipeline traversal by gathering all the promises and awaiting them all | |||
| // see discussion https://github.com/pixiebrix/pixiebrix-extension/pull/4013#discussion_r944690969 | |||
| private readonly nunjuckValidationPromises: Array<Promise<void>> = []; | |||
There was a problem hiding this comment.
Comment copy-pasted from existing code
| @@ -263,7 +263,7 @@ export function removeExtensions(extensionIds: UUID[]): void { | |||
| console.debug("sidebarController:removeExtensions", { extensionIds }); | |||
|
|
|||
| // `panels` is const, so replace the contents | |||
| const current = panels.splice(0, panels.length); | |||
| const current = panels.splice(0); | |||
There was a problem hiding this comment.
Nit: This is the default
I'm not a fan of splice in general so I'd be open to adding a cloneAndClearArray utility to make the intent clear.
| const current = panels.splice(0); | |
| const current = cloneAndClearArray(panels); |
| url: expression, | ||
| }, | ||
| }), | ||
| ]; |
There was a problem hiding this comment.
I copied this updated test format from requestPermissionAnalysis.test.ts
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
The validation successfully goes through the sandbox, but it requires changes to the analysis in order to accept async validation. I think that's a larger task that can be done separately from this PR.
Tasks
Test strings
{% %}reports a template error correctly{%reports a null reference error inside nunjucks itself, it's not our bugFuture work
Checklist
src/tsconfig.strictNullChecks.json(if possible)