Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement TransformError/TemplateError subclasses #89

Merged
merged 3 commits into from
Dec 7, 2024

Conversation

noelforte
Copy link
Contributor

Ok! Here's the PR for the errors.ts file. I tried to be as conservative as possible with my edits and did a lot of research on the best way to implement 'default' properties in class declarations in TypeScript. I'm still pretty new to coding in TypeScript so I'm happy to make any corrections or edits if you would prefer things be handled differently.

Some quick notes to summarize things we discussed previously:

  • I opted not to expose a code property on TemplateError instances since I think good error tracing can help identify the problem.

  • Likewise, I removed redundant logging of the cause message from TemplateError implementations, instead only including a short message with cause.name if it's defined. The TransformError message only adds context about the compiled JS function so I think it's better left in the stack.

    A `TransformError` now looks like this:
    error: Uncaught (in promise) TemplateError: Error in template test.vto:1:1
    
    {{ foo 'alpha', 'beta', 'gamma' }}
    
    (via TransformError)
    
              throw new TemplateError(path, source, cause.position, cause);
                    ^
        at Environment.compile (file:///Users/noel/Developer/projects/external/vento/src/environment.ts:144:17)
        at Environment.load (file:///Users/noel/Developer/projects/external/vento/src/environment.ts:208:29)
        at eventLoopTick (ext:core/01_core.js:175:7)
        at async file:///Users/noel/Developer/tests/vento-transformations/run.js:5:18
    Caused by: TransformError: [meriyah] [2:26-2:33]: Expected ')' while parsing compiled template function:
    
    2 __exports.content += (foo 'alpha', 'beta', 'gamma') ?? "";
                              ^
        at transformTemplateCode (file:///Users/noel/Developer/projects/external/vento/src/transformer.ts:150:11)
        at Environment.compile (file:///Users/noel/Developer/projects/external/vento/src/environment.ts:141:16)
        at Environment.load (file:///Users/noel/Developer/projects/external/vento/src/environment.ts:208:29)
        at eventLoopTick (ext:core/01_core.js:175:7)
        at async file:///Users/noel/Developer/tests/vento-transformations/run.js:5:18
  • I added an argument to the Environment Function constructor to expose TemplateError class to the compiled template. I tried adding it to the Environment class (new this.error) but since TemplateError is hoisted it seemed cleaner to use it as a global.

  • Moved errorLine to errors.ts and exported it so it can be used elsewhere. Not sure if you want errors.ts to be included as a module entrypoint.

Let me know if there's any corrections I can make! Thanks for taking a look.

Copy link
Collaborator

@oscarotero oscarotero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. Your changes look great!
I just left a question.

src/errors.ts Show resolved Hide resolved
@noelforte
Copy link
Contributor Author

Renamed VentoError to VentoBaseError (could also just be BaseError) which only overrides assignment of the error name so it doesn't need to be assigned explicitly. Because Error.constructor isn't overwritten, it can be reused on subclasses. As for calls to Error.captureStackTrace() you're correct that Error does this already; it was a lack of understanding on my part that I included it.

@oscarotero oscarotero merged commit 89ca698 into ventojs:main Dec 7, 2024
1 check passed
@oscarotero
Copy link
Collaborator

Great job! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants