Improve transform error handling with compiled function annotations

[noelforte]

Here's an attempt to further improve #85. I'm not super happy with the implementation, since it feels like transform errors and template errors should be handled seperately. I didn't want to start refactoring everything so I tried to make these edits as reviewable as possible and keep the discussion going.

My reasoning for adding an annotated message logging the line of the compiled template function was to explain the errors that meriyah might throw and make it clear that the error is happening on a compiled function, not a user-authored one. To deal with these errors originating from the Eleventy integration, I also plan to add a message to my plugin reminding users to check for undeclared shortcodes and filters since those will cause errors when meriyah goes to parse/transform.

Happy to edit/revise further if there's a cleaner way of tracking these errors!

---

Also included in this pull:

• 871cb1b adds type annotation for ESTree nodes so Typescript can pick those up.
• 5279c7d patches a build error I was getting with dnt and JSR, for whatever reason, dnt was complaining that @std/[email protected] wasn't able to be resolved. Reverting to the deno.land url fixes this for now.

[oscarotero]

Hi and sorry for the late response!
Your changes look nice, but now the error only shows the compiled code, not the original template code, and this will be confusing for some users.

For example, if I have this code:

<h1>
{{ foo() }}
</h1>

The error that I see is this:

error: Uncaught (in promise) Error: Error in the template demo/template.vto:2:1

{{ foo() }}

> it.foo is not a function

And this is useful, because I can see the line of the vento file where the error has originated.

But if I have a parse error like this (Note the missing final parenthesis):

<h1>
{{ foo( }}
</h1>

The error I see is this:

error: Uncaught (in promise) SyntaxError: Failed to parse template function internally. <template>:[3:33-3:34]: Expected ')'

__exports.content += (foo() ?? "";

And the origin of the error is not clear, I can't see the filename or the line number of the vento code.

As an idea, instead of throwing directly the error of the compiled code, it can wrapped with a vento error, passing the meriyah error as the cause. This would show the two errors in the console: the vento code and the compiled code.

What do you think? can you try these changes?

[noelforte]

now the error only shows the compiled code

Shoot, I thought I included the original code. Will incorporate that and try out some of your suggestions. Thanks for taking a look!

[noelforte]

Just checked on my end with the changes I initially comitted and was able to confirm that errors were getting thrown with original code, strange you weren't seeing that.

Anyway, just pushed new changes. Quick summary:

• Refactored the position-grabbing logic to use a regex and Number constructor in transformer.ts, unsure if you were purposefully avoiding regex matching
• Created a class TransformError to handle graceful parsing of the error meriyah throws, add pos as an property to instances of that error, then catch from environment.ts and throw accordingly via instanceof.

I mentioned before; not sure if moving all error-making things to an errors.ts file is worth it? Even if env.createError needs to exist on Environment for errors thrown via __env.createError, then a subclass constructor could always be exposed on Environment.

// environment.ts
import { RuntimeError } from './errors.ts`

class Environment {
// ...
createError = 
}

Maybe ... extends Error patterns are overkill. Just an idea. Hope this is at least a bit cleaner!

[oscarotero]

I just checked it and the new errors are great, much more clear!

Refactored the position-grabbing logic to use a regex and Number constructor in transformer.ts, unsure if you were purposefully avoiding regex matching

I tend to avoid regex if a simple indexof and similar functions do the job (for performance and reduce complexity). But your changes are okay, no problem. Also, this code is run only on error, so performance here doesn't matter.

My only doubt is whether the other caused errors should be added to the error chain. The top error shows the messages of the nested errors nicely formatted, for example:

error: Uncaught (in promise) Error: Error in the template demo/template.vto:2:1

{{ foo() : }}

> Error transforming template function.
> [3:28-3:29]: Expected ')' while transforming:
> 
> __exports.content += (foo() :) ?? "";
>                             ^

    return new Error(
           ^
    at Environment.createError (file:///Users/oscarotero/www/ventojs/vento/src/environment.ts:326:12)
    at Environment.compile (file:///Users/oscarotero/www/ventojs/vento/src/environment.ts:143:22)
    at Environment.load (file:///Users/oscarotero/www/ventojs/vento/src/environment.ts:206:29)
    at eventLoopTick (ext:core/01_core.js:175:7)
    at async Environment.run (file:///Users/oscarotero/www/ventojs/vento/src/environment.ts:76:22)
    at async file:///Users/oscarotero/www/ventojs/vento/demo/main.ts:18:16
Caused by: TransformError: Error transforming template function.
[3:28-3:29]: Expected ')' while transforming:

__exports.content += (foo() :) ?? "";
                            ^
    at transformTemplateCode (file:///Users/oscarotero/www/ventojs/vento/src/transformer.ts:158:11)
    at Environment.compile (file:///Users/oscarotero/www/ventojs/vento/src/environment.ts:140:16)
    at Environment.load (file:///Users/oscarotero/www/ventojs/vento/src/environment.ts:206:29)
    at eventLoopTick (ext:core/01_core.js:175:7)
    at async Environment.run (file:///Users/oscarotero/www/ventojs/vento/src/environment.ts:76:22)
    at async file:///Users/oscarotero/www/ventojs/vento/demo/main.ts:18:16
Caused by: SyntaxError: [3:28-3:29]: Expected ')'
    at t (file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:9885)
    at ee (file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:70045)
    at file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:101763
    at Ye (file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:102147)
    at Oe (file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:91461)
    at je (file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:92031)
    at file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:87041
    at ye (file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:87129)
    at Te (file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:80326)
    at xe (file:///Users/oscarotero/Library/Caches/deno/npm/registry.npmjs.org/meriyah/6.0.3/dist/meriyah.min.mjs:1:79246)

The other Caused errors only repeat the same error message and the backtrace to meriyah files maybe are unnecesary.
I'm not sure, maybe it's useful for other errors, just want to know your opinion.

[noelforte]

The stack might be more useful if meriyah wasn't distributed as a minified library. Since it is, I agree with you—it's better left up to the consuming packages to handle errors more gracefully.

The just pushed changes should address this and also unify other errors thrown in transformer.ts. I also applied some defaults to createError so the problem template can be identified wherever possible, even if it's empty.

Couple questions:

1. What are your thoughts on creating a class of error (ie TemplateError) and/or defining all error object shapes in a separate file? I can explore what that would look like and open another pull for review if you're open.
2. With or without the above, I'm thinking ahead to how I can expose these errors when integrated with Eleventy. Would it be possible to expose the template code snippet on the error object so it could be absorbed by implementations?

[oscarotero]

Hi!

What are your thoughts on creating a class of error (ie TemplateError) and/or defining all error object shapes in a separate file? I can explore what that would look like and open another pull for review if you're open.

Yes, it makes sense.

With or without the above, I'm thinking ahead to how I can expose these errors when integrated with Eleventy. Would it be possible to expose the template code snippet on the error object so it could be absorbed by implementations?

Do you mean a new property in the error with the vento code? If this helps to Eleventy integration, yes, why not.

[oscarotero]

@noelforte Is this PR is ready to be merged?

[noelforte]

@oscarotero Sure go ahead. I started working on an errors.ts file in another branch that I'll raise a separate PR for. In the meantime, I just rebase/force-pushed edits that clean up the commit history and are good to go. 👍

[oscarotero]

great. thank you!