twig.js icon indicating copy to clipboard operation
twig.js copied to clipboard

renderFile method swallows errors

Open cbovis opened this issue 7 years ago • 14 comments

Calling Twig.renderFile with an invalid file path results in the callback not being executed.

For example,

  try {
    Twig.renderFile(templatePath, model, (err, html) => {
      console.log(err);
      console.log(html);
    });
  } catch (err) {
    console.log(err);
  }

If templatePath is incorrect then the specified callback never gets fired and no error is thrown either meaning I have no way to handle errors.

cbovis avatar Nov 14 '17 22:11 cbovis

In https://github.com/twigjs/twig.js/blob/master/src/twig.exports.js#L162, add a new property to the params object: https://github.com/twigjs/twig.js/blob/master/src/twig.exports.js#L173 called error made up of a function that takes an err param. In that function, call fn with the err param.

Make a PR and I'll merge.

dave-irvine avatar Nov 15 '17 08:11 dave-irvine

If you use the synchonous read with "fs.statSync" by using the following option:

let params = {
     settings: { "twig options": { async: false} }
};

Twig.renderFile( "xyz", params, console.log);

Then all errors will be thrown correctly. The problem seems to be that the callback for:

https://github.com/twigjs/twig.js/blob/master/src/twig.loader.fs.js#L50

is never beeing called, so passing a func for "error_callback" in:

https://github.com/twigjs/twig.js/blob/master/src/twig.loader.fs.js#L52

seems not to solve the problem...

kindziora avatar Feb 08 '18 10:02 kindziora

@dave-irvine it's still affecting

cojack avatar Jan 25 '19 14:01 cojack

is there any work around for this?

r3wt avatar Dec 24 '19 22:12 r3wt

@r3wt yes, use twing instead, is better written

cojack avatar Dec 28 '19 18:12 cojack

+100

Having the same issue here. To my big surprise this very important issue still isn't fixed 4 years later (!!), now in 2021... While it's such a minor, but important thing to fix.

Sorry, but async is completly useless when the callback doesn't set the error.

Hopefully Twing does this better.

Friksel avatar Jan 30 '21 17:01 Friksel

@Friksel This is an open source project. Feel free to contribute and fix this issue!

MickL avatar Jan 30 '21 17:01 MickL

@Friksel if its such a minor fix, I look forward to your open source contribution.

I believe I actually wrote instructions for how to fix this in 2017, yet nobody ever made a pull request implementing my suggestion.

dave-irvine avatar Jan 30 '21 17:01 dave-irvine

It takes you four years to fix an issue that shouldn't be there in the first place, but you react in 10 seconds to somebody pointing out that the issue isn't fixed and expect him to fix your error? wow. great responsible attitude.

Friksel avatar Jan 30 '21 17:01 Friksel

Checking through your comment history on other projects, I can see you are not someone I want to interact with.

Please enjoy open source responsibly.

dave-irvine avatar Jan 30 '21 18:01 dave-irvine

@Friksel The reason it isn't fixed is because twig.js, and other templating libraries have pretty much run their course. in the modern day, we have react which is used on the frontend, so really the only use for twig.js these days is maybe for email templating. its easy to see why there wasn't enough developer need to fix this, as its simply a non factor as long as the template exists.

I will disagree with you though @dave-irvine blocking @Friksel over these minor comments was a bit of an overreaction on your part. I would kindly ask you to cool off and consider rescinding the ban.

I would also like to ask @Friksel to remain respectful in the future when commenting. As someone who has been hotheaded on github plenty of times, i can tell you its not a good look for employers when evaluating your resume. They do due diligence on candidates including investigating their behavior on github and stack overflow, so try to keep that in mind when using these platforms.

r3wt avatar Jan 30 '21 20:01 r3wt

I'm agreeing with @dave-irvine on this matter. Maintaining open source software requires a lot of effort, least of which is community management. No tolerance for ignorant people in the issue queue. We aren't working on twig.js full time, so have empathy.

To address your second point, I would hire dave-irvine because of this action. It shows leadership and a desire to keep things positive in a team setting.

RobLoach avatar Jan 30 '21 20:01 RobLoach

To address your second point, I would hire dave-irvine because of this action. It shows leadership and a desire to keep things positive in a team setting.

2 things:

  1. that was my 3rd point
  2. it was addressed to @Friksel

To be fair, i could have started a new paragraph to make it clearer. i haven't really thought about the rules of writing for some time, thanks for the reminder.

r3wt avatar Jan 30 '21 21:01 r3wt

Hi @r3wt, I appreciate you leaving feedback on this situation.

Regarding your thoughts on whether I overreacted; I have little to no acceptance of toxicity in the open-source community, the Internet is a cesspool of toxicity as it is, and I see no reason to subject myself to it in places where I try to conduct my work. In this particular case, while the two comments the user made in isolation may not seem particularly toxic, I had made a further investigation into the user's other comments on open source projects and made my decision based on this.

Regarding your thoughts on rescinding the block; as I am not the only maintainer on this project I opened a secondary issue to discuss this block here: https://github.com/twigjs/twig.js/issues/772 and left it to the other maintainers to decide if I had overreacted in this case and whether they wished to remove the block.

In truth this is the first time I've had to take such an action, let alone in the Twig.js community. Its possible we may need community guidelines to deal with future issues, but in retrospect I would probably take the same action again. Toxicity has no place in open source. End of story IMO.

dave-irvine avatar Jan 30 '21 22:01 dave-irvine