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

renderFile method swallows errors #509

Closed
cbovis opened this issue Nov 14, 2017 · 14 comments · Fixed by #851
Closed

renderFile method swallows errors #509

cbovis opened this issue Nov 14, 2017 · 14 comments · Fixed by #851

Comments

@cbovis
Copy link

cbovis commented Nov 14, 2017

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.

@dave-irvine
Copy link
Contributor

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.

@kindziora
Copy link

kindziora commented Feb 8, 2018

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...

@cojack
Copy link

cojack commented Jan 25, 2019

@dave-irvine it's still affecting

@r3wt
Copy link

r3wt commented Dec 24, 2019

is there any work around for this?

@cojack
Copy link

cojack commented Dec 28, 2019

@r3wt yes, use twing instead, is better written

@Friksel

This comment was marked as abuse.

@MickL
Copy link

MickL commented Jan 30, 2021

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

@dave-irvine
Copy link
Contributor

@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.

@Friksel

This comment was marked as abuse.

@dave-irvine
Copy link
Contributor

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.

@r3wt
Copy link

r3wt commented Jan 30, 2021

@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.

@RobLoach
Copy link
Collaborator

RobLoach commented Jan 30, 2021

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.

@r3wt
Copy link

r3wt commented Jan 30, 2021

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.

@dave-irvine
Copy link
Contributor

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: #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.

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

Successfully merging a pull request may close this issue.

9 participants