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.

Sign up for GitHub

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

Fix: Uncaught exceptions cause a silent exit #721

Merged
merged 4 commits into from
Feb 25, 2019
Merged

Fix: Uncaught exceptions cause a silent exit #721

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a762a8
Show errors on console
amad-person Feb 21, 2019
669325b
List winston options in alphabetical order
amad-person Feb 21, 2019
154e07c
Move second arg to new line in fileContent renderString
amad-person Feb 21, 2019
ab39039
Update logger
amad-person Feb 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/lib/markbind/src/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,8 @@ Parser.prototype._preprocess = function (node, context, config) {

// Render inner file content
fileContent = nunjucks.renderString(fileContent,
{ ...pageVariables, ...includeVariables, ...userDefinedVariables });
{ ...pageVariables, ...includeVariables, ...userDefinedVariables },
{ path: actualFilePath });

// Delete variable attributes in include
Object.keys(element.attribs).forEach((attribute) => {
Expand Down Expand Up @@ -504,7 +505,8 @@ Parser.prototype.includeFile = function (file, config) {
const { parent, relative } = calculateNewBaseUrls(file, config.rootPath, config.baseUrlMap);
const userDefinedVariables = config.userDefinedVariablesMap[path.resolve(parent, relative)];
const pageVariables = extractPageVariables(path.basename(file), data, userDefinedVariables, {});
const fileContent = nunjucks.renderString(data, { ...pageVariables, ...userDefinedVariables });
const fileContent = nunjucks.renderString(data, { ...pageVariables, ...userDefinedVariables },
{ path: actualFilePath });
const fileExt = utils.getExt(file);
if (utils.isMarkdownFileExt(fileExt)) {
context.source = 'md';
Expand Down Expand Up @@ -658,7 +660,7 @@ Parser.prototype._rebaseReference = function (node, foundBase) {
// and let the hostBaseUrl value be injected later.
hostBaseUrl: '{{hostBaseUrl}}',
baseUrl: newBaseUrl,
});
}, { path: filePath });
element.children = cheerio.parseHTML(rendered, true);
cheerio.prototype.options.xmlMode = true;
}
Expand Down Expand Up @@ -694,7 +696,7 @@ Parser.prototype._rebaseReferenceForStaticIncludes = function (pageData, element

const newBase = fileBase.relative;
const newBaseUrl = `{{hostBaseUrl}}/${newBase}`;
return nunjucks.renderString(pageData, { baseUrl: newBaseUrl });
return nunjucks.renderString(pageData, { baseUrl: newBaseUrl }, { path: filePath });
};

module.exports = Parser;
7 changes: 7 additions & 0 deletions src/util/logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@ const DailyRotateFile = require('winston-daily-rotate-file');
winston.configure({
exitOnError: false,
transports: [
new (winston.transports.Console)({
colorize: true,
handleExceptions: true,
humanReadableUnhandledException: true,
level: 'error',
showLevel: true,
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this different from console.error? Should we remove that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean from this line:

markbind/src/lib/markbind/src/parser.js

Line 82 in 1686253

this._onError = this._options.errorHandler || console.error;

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this line:

markbind/src/util/logger.js

Line 33 in 669325b

console.log(chalk.red(`error: ${text}`));

Copy link
Contributor Author

@amad-person amad-person Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The current mechanism of logging is as follows:

We are using logger.js functions to print messages to the console with console.log() statements and a winston-daily-rotate-file transport set to level debug that writes messages to a log file.

Nunjucks errors were not being logged to the console because we are not calling logger.error() explicitly in nunjucks.renderString() (as we don't have error handlers for these calls). Other errors do get printed to the console because we call logger.error() when we catch them.

However, nunjucks errors were being printed to the log file because of the winston-daily-rotate-file transport.

If we want to remove console.error (and other console.log statements in logger.js), then we should use the winston console transport with the level set to debug. There is an option to use custom formats (color, etc.) too.

Copy link
Member

@yamgent yamgent Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Correct me if I am wrong, nunjucks is actually using winston-daily-rotate-file internally? I am struggling to see how the pieces are connected here.

Copy link
Contributor Author

@amad-person amad-person Feb 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Our application does use winston through the logger, but we have also configured winston to handle logging uncaught exceptions.

markbind/src/util/logger.js

Line 15 in 6bbe1fa

handleExceptions: true,

That's how nunjucks errors were being logged to the log file, because we don't have error handlers for the renderString() calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Thanks for checking this. It answers the question originally raised by @yamgent.

  2. How about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Just tested this.

    // index.js

logger.error('hi');
    console.log(chalk.red(`error: ${text}`)); // console.log(chalk.red(`error: ${text}`));
    screenshot 2019-02-23 16 01 11 screenshot 2019-02-23 15 54 45

    Let's set winston.transports.Console level to 'info' and then remove console.log in logger.error, logger.warn and logger.info.

Sorry for the confusion regarding the console.error reference! I didn't realise logger.error was using console.log.

Copy link
Contributor Author

@amad-person amad-person Feb 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Let's set winston.transports.Console level to 'info' and then remove console.log in logger.error, logger.warn and logger.info.

May I do these changes in this PR itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please.

new DailyRotateFile({
datePattern: 'YYYY-MM-DD',
dirname: '_markbind/logs',
Expand Down