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

Invalid/unclosed FOR loops yield no errors #322

Closed
davidjb opened this issue Jul 31, 2023 · 6 comments
Closed

Invalid/unclosed FOR loops yield no errors #322

davidjb opened this issue Jul 31, 2023 · 6 comments

Comments

@davidjb
Copy link
Contributor

davidjb commented Jul 31, 2023

If a mistake is made within FOR loop syntax, due to either a missing or incorrect END-FOR command, no errors are produced when calling createReport.

Examples

  • A small reproducible example consisting of the code and .docx template you used.

Each of these examples yields the same screenshot .docx result and explanation below.

Example 1:

Before
{for person in people}{$person}{end-for persons}
After

Example 2:

Before
{for person in people}{$person}{end-for}
After

Example 3:

Before
{for person in people}
After

Screenshot and explanation

  • A screenshot of the result you get and an explanation as to why this result is unexpected.

image

No errors are thrown nor any in-document errors appear with failFast: true. Anything in the document after the broken loop will not appear (e.g. the After text).

In the examples above, it would be useful to:

  • Get an error message on an incomplete FOR statement (like an IncompleteConditionalStatementError for IF without an END-IF)
  • Be able to close a FOR loop without needing to name its target. An invalid END-FOR that closes an unknown loop would still error though to avoid subtle bugs (e.g. example 1 working and the invalid variable name persons being ignored).

Compiler and toolchain

  • Information about your compiler toolchain version (e.g. Angular, Webpack, etc.): Directly testing with ESM/Node script:

    import * as fs from 'node:fs/promises';
    import * as docx from 'docx-templates';
    
    try {
      const result = await docx.createReport({
        template: await fs.readFile(new URL('./input.docx', import.meta.url)),
        cmdDelimiter: ['{', '}'],
        failFast: false,
        data: {
          people: ["Jane", "Sally", "Tom"],
        }
      })
      await fs.writeFile(new URL('./output.docx', import.meta.url), result)
      console.log("Wrote out output.docx fine")
    } catch (errs) {
      console.error(errs)
    }

Runtime

  • Information about your runtime environment (which browser, Electron, or NodeJS version): NodeJS v20.4.0, using docx-templates master from Git
@jjhbw
Copy link
Collaborator

jjhbw commented Jul 31, 2023

Good find, and thanks for the clear writeup. This is a duplicate of #180 but with better context and documentation.

I think the easiest solution for this would be to throw an error if the 'tree walking' of the document concludes without encountering an END-FOR. I'll take a look to see how difficult this is going to be... Let me know if you have any ideas!

jjhbw added a commit that referenced this issue Jul 31, 2023
@jjhbw
Copy link
Collaborator

jjhbw commented Jul 31, 2023

I think I have it. See af8ab37.

Being able to close a FOR loop without naming its target is too complicated for now. I also think that 'naming' the FOR loop that you are terminating helps readability, but that is very subjective.

@davidjb
Copy link
Contributor Author

davidjb commented Aug 1, 2023

Thanks for looking at this @jjhbw, really appreciate your efforts 👍

I tried out the latest master, which includes that commit, and it seems there's a regression. A document like:

This is {if true}working

produces two errors now:

[
  [Error: Incomplete IF/END-IF statement. Make sure each IF-statement has a corresponding END-IF command.],
  [Error: Unterminated FOR-loop ('FOR __if_0'). Make sure each FOR loop has a corresponding END-FOR command.]
]

This bit of the code

ctx.loops.push({
refNode: node,
refNodeLevel: ctx.level,
varName,
loopOver,
isIf,
// run through the loop once first, without outputting anything
// (if we don't do it like this, we could not run empty loops!)
idx: -1,
});
}
seems to be pushing IF commands into the ctx.loops array, which causes the error to be thrown.

jjhbw added a commit that referenced this issue Aug 3, 2023
…ing END-IF` should result in only the `Incomplete IF/END-IF statement` error, but it currently also erroneously results in the `Unterminated FOR-loop ('FOR __if_0')` error.
jjhbw added a commit that referenced this issue Aug 3, 2023
…also returning `Unterminated FOR-loop ('FOR __if_0')` when `failFast==false`.
@jjhbw
Copy link
Collaborator

jjhbw commented Aug 3, 2023

Good find, that is definitely not intentional. Had no idea loops and IF statements were intertwined like this... Reproduced in 15e006d and fixed in b0a0f7f.

Let me know what you think!

@jjhbw jjhbw closed this as completed Aug 8, 2023
@jjhbw
Copy link
Collaborator

jjhbw commented Aug 8, 2023

Released fix in https://github.com/guigrpa/docx-templates/releases/tag/v4.11.3

@davidjb
Copy link
Contributor Author

davidjb commented Aug 14, 2023

Looks good to me, thanks @jjhbw - really appreciate your work 👍

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

No branches or pull requests

2 participants