eta-dev / eta

Embedded JS template engine for Node, Deno, and the browser. Lighweight, fast, and pluggable. Written in TypeScript
https://eta.js.org
MIT License
1.35k stars 60 forks source link

feat: expose error type #276

Closed multivoltage closed 3 months ago

multivoltage commented 4 months ago

I tried to export EtaError types introducing new types like @nebrelbug did in THIS issue.

Some point about he PR:

I don't know if this PR follow exactly the structure described here: https://github.com/eta-dev/eta/issues/243#issuecomment-1646243319

But probably this PR can be a starting point and I'm happy to change some part after review

nebrelbug commented 3 months ago

@multivoltage thanks for the PR! This looks great. Just a few changes before I can merge:

I appreciate the TypeScript updates!

nebrelbug commented 3 months ago

Also if you can make sure to follow the Commitlint format for commit messages, that's what's causing tests to fail right now. Thanks!!

multivoltage commented 3 months ago
npx commitlint --from ee0a1d44addd162b53b20329799b93628fcba22e~6 --to ee0a1d44addd162b53b20329799b93628fcba22e --verbose
⧗   input: feat: export EtaNameResolutionError
✔   found 0 problems, 0 warnings
⧗   input: feat: add use case for EtaFileResolutionError
✔   found 0 problems, 0 warnings
⧗   input: feat: export EtaFileResolutionError
✔   found 0 problems, 0 warnings
⧗   input: feat: use custom error for compile() api
✔   found 0 problems, 0 warnings
⧗   input: feat: export EtaRuntimeError
✔   found 0 problems, 0 warnings
⧗   input: feat: export EtaParseError
✔   found 0 problems, 0 warnings

In my local pc seems ok (node 18)

nebrelbug commented 3 months ago

@multivoltage hmm, you're right. I'm not actually sure what the problem is, but it seems like it may be a problem with the version of commitlint that the GitHub Action is installing.

multivoltage commented 3 months ago

@nebrelbug I replicated issue. In. my local pc like I said I run "npx commitlint....".

But in you yaml you install before

      - name: Install commitlint
        run: |
          npm install conventional-changelog-conventionalcommits
          npm install commitlint@latest

and then use npx.

I think you have 2 possible fix:

  1. remove step to install the two deps and keep going to use "npx commitlint" like in my local pc.
  2. Installing same deps in my pc and run "npx commitlint" the error is the same. I solved running also @commitlint/format

Please give me a feedback. Do you plan to fix this in seperate pr?

nebrelbug commented 3 months ago

@multivoltage I'll fix the commitlint issue in a separate PR later. In the meantime, I'll merge anyways if you can fix the issues I mentioned above.

multivoltage commented 3 months ago

@nebrelbug Sorry. I did not notice your first comment :)

I think it would be great if all Eta errors extended a base class EtaError, which could just be declared like class EtaError extends Error {}.

I thought the same thing inirtially but then I ask my self?

What EtaError give me more than a basic Erro? Anyway I will do it :)

nebrelbug commented 3 months ago

@multivoltage thank you! Having a base EtaError class will be useful if people want to catch all errors that Eta throws.

nebrelbug commented 3 months ago

@multivoltage released in Eta 3.4.0:)