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

Reorganise variable processing #1189

Merged
merged 2 commits into from
Jun 9, 2020
Merged

Reorganise variable processing #1189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7040ac8
Reorganise variable processing (part 1)
ang-zeyu May 30, 2020
d6a00b0
Reorganise variable processing (part 2)
ang-zeyu May 20, 2020
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
46 changes: 20 additions & 26 deletions src/Page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class Page {
* @property {string} pageTemplate template used for this page
* @property {string} title
* @property {string} titlePrefix https://markbind.org/userGuide/siteConfiguration.html#titleprefix
* @property {Object<string, any>} userDefinedVariablesMap
* @property {VariablePreprocessor} variablePreprocessor
* @property {string} sourcePath the source file for rendering this page
* @property {string} tempPath the temp path for writing intermediate result
* @property {string} resultPath the output path of this page
Expand Down Expand Up @@ -170,9 +170,9 @@ class Page {
*/
this.titlePrefix = pageConfig.titlePrefix;
/**
* @type {Object<string, any>}
* @type {VariablePreprocessor}
*/
this.userDefinedVariablesMap = pageConfig.userDefinedVariablesMap;
this.variablePreprocessor = pageConfig.variablePreprocessor;

/**
* The source file for rendering this page
Expand Down Expand Up @@ -631,10 +631,9 @@ class Page {
}
// Set header file as an includedFile
this.includedFiles.add(headerPath);
// Map variables
const parentSite = urlUtils.getParentSiteAbsolutePath(this.sourcePath, this.rootPath, this.baseUrlMap);
const userDefinedVariables = this.userDefinedVariablesMap[parentSite];
return `${njUtil.renderRaw(headerContent, userDefinedVariables)}\n${pageData}`;

const renderedHeader = this.variablePreprocessor.renderSiteVariables(this.sourcePath, headerContent);
return `${renderedHeader}\n${pageData}`;
}

/**
Expand All @@ -661,10 +660,9 @@ class Page {
const footerContent = fs.readFileSync(footerPath, 'utf8');
// Set footer file as an includedFile
this.includedFiles.add(footerPath);
// Map variables
const parentSite = urlUtils.getParentSiteAbsolutePath(this.sourcePath, this.rootPath, this.baseUrlMap);
const userDefinedVariables = this.userDefinedVariablesMap[parentSite];
return `${pageData}\n${njUtil.renderRaw(footerContent, userDefinedVariables)}`;

const renderedFooter = this.variablePreprocessor.renderSiteVariables(this.sourcePath, footerContent);
return `${pageData}\n${renderedFooter}`;
}

/**
Expand Down Expand Up @@ -695,10 +693,7 @@ class Page {
}
this.includedFiles.add(siteNavPath);

// Render variables
const parentSite = urlUtils.getParentSiteAbsolutePath(this.sourcePath, this.rootPath, this.baseUrlMap);
const userDefinedVariables = this.userDefinedVariablesMap[parentSite];
const siteNavMappedData = njUtil.renderRaw(siteNavContent, userDefinedVariables);
const siteNavMappedData = this.variablePreprocessor.renderSiteVariables(this.sourcePath, siteNavContent);

// Check navigation elements
const $ = cheerio.load(siteNavMappedData);
Expand Down Expand Up @@ -879,11 +874,9 @@ class Page {
const headFileContent = fs.readFileSync(headFilePath, 'utf8');
// Set head file as an includedFile
this.includedFiles.add(headFilePath);
// Map variables
const parentSite = urlUtils.getParentSiteAbsolutePath(this.sourcePath, this.rootPath, this.baseUrlMap);
const userDefinedVariables = this.userDefinedVariablesMap[parentSite];
const headFileMappedData = njUtil.renderRaw(headFileContent, userDefinedVariables)
.trim();

const headFileMappedData = this.variablePreprocessor.renderSiteVariables(this.sourcePath,
headFileContent).trim();
// Split top and bottom contents
const $ = cheerio.load(headFileMappedData, { xmlMode: false });
if ($('head-top').length) {
Expand Down Expand Up @@ -941,23 +934,23 @@ class Page {
* @typedef {Object<string, any>} FileConfig
* @property {Set<string>} baseUrlMap the set of urls representing the sites' base directories
* @property {string} rootPath
* @property {Object<string, any>} userDefinedVariablesMap
* @property {VariablePreprocessor} variablePreprocessor
* @property {Object<string, number>} headerIdMap
* @property {boolean} fixedHeader indicates whether the header of the page is fixed
*/

generate(builtFiles) {
this.includedFiles = new Set([this.sourcePath]);
this.headerIdMap = {}; // Reset for live reload

const markbinder = new MarkBind();
const markbinder = new MarkBind({
variablePreprocessor: this.variablePreprocessor,
});
/**
* @type {FileConfig}
*/
const fileConfig = {
baseUrlMap: this.baseUrlMap,
rootPath: this.rootPath,
userDefinedVariablesMap: this.userDefinedVariablesMap,
headerIdMap: this.headerIdMap,
fixedHeader: this.fixedHeader,
};
Expand Down Expand Up @@ -1264,11 +1257,12 @@ class Page {
* We create a local instance of Markbind for an empty dynamicIncludeSrc
* so that we only recursively rebuild the file's included content
*/
const markbinder = new MarkBind();
const markbinder = new MarkBind({
variablePreprocessor: this.variablePreprocessor,
});
return fs.readFileAsync(dependency.to, 'utf-8')
.then(result => markbinder.includeFile(dependency.to, result, {
baseUrlMap: this.baseUrlMap,
userDefinedVariablesMap: this.userDefinedVariablesMap,
rootPath: this.rootPath,
cwf: file,
}))
Expand Down
47 changes: 19 additions & 28 deletions src/Site.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const path = require('path');
const Promise = require('bluebird');
const ProgressBar = require('progress');
const walkSync = require('walk-sync');
const MarkBind = require('./lib/markbind/src/parser');
const VariablePreprocessor = require('./lib/markbind/src/preprocessors/variablePreprocessor');
const njUtil = require('./lib/markbind/src/utils/nunjuckUtils');
const injectHtmlParser2SpecialTags = require('./lib/markbind/src/patches/htmlparser2');
const injectMarkdownItSpecialTags = require(
Expand Down Expand Up @@ -134,7 +134,9 @@ class Site {
*/
this.siteConfig = undefined;
this.siteConfigPath = siteConfigPath;
this.userDefinedVariablesMap = {};

// Site wide variable preprocessor
this.variablePreprocessor = undefined;

// Lazy reload properties
this.onePagePath = onePagePath;
Expand Down Expand Up @@ -271,7 +273,7 @@ class Site {
title: config.title || '',
titlePrefix: this.siteConfig.titlePrefix,
headingIndexingLevel: this.siteConfig.headingIndexingLevel,
userDefinedVariablesMap: this.userDefinedVariablesMap,
variablePreprocessor: this.variablePreprocessor,
sourcePath,
resultPath,
asset: {
Expand Down Expand Up @@ -513,6 +515,7 @@ class Site {
.map(x => path.resolve(this.rootPath, x));

this.baseUrlMap = new Set(candidates.map(candidate => path.dirname(candidate)));
this.variablePreprocessor = new VariablePreprocessor(this.rootPath, this.baseUrlMap);

return Promise.resolve();
}
Expand All @@ -523,12 +526,9 @@ class Site {
collectUserDefinedVariablesMap() {
// The key is the base directory of the site/subsites,
// while the value is a mapping of user defined variables
this.userDefinedVariablesMap = {};
const markbindVariable = { MarkBind: MARKBIND_LINK_HTML };
this.variablePreprocessor.resetUserDefinedVariablesMap();

this.baseUrlMap.forEach((base) => {
const userDefinedVariables = {};
Object.assign(userDefinedVariables, markbindVariable);
const userDefinedVariablesPath = path.resolve(base, USER_VARIABLES_PATH);
const userDefinedVariablesDir = path.dirname(userDefinedVariablesPath);
let content;
Expand All @@ -539,34 +539,31 @@ class Site {
logger.warn(e.message);
}

// This is to prevent the first nunjuck call from converting {{baseUrl}} to an empty string
// and let the baseUrl value be injected later.
userDefinedVariables.baseUrl = '{{baseUrl}}';
this.userDefinedVariablesMap[base] = userDefinedVariables;
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of arrow number 1 in the diagram (site variable extraction), similar ones follow in this file

This is to prevent the first nunjuck call from converting {{baseUrl}} to an empty string
and let the baseUrl value be injected later.
*/
this.variablePreprocessor.addUserDefinedVariable(base, 'baseUrl', '{{baseUrl}}');
this.variablePreprocessor.addUserDefinedVariable(base, 'MarkBind', MARKBIND_LINK_HTML);

const $ = cheerio.load(content);
$('variable,span').each(function () {
const name = $(this).attr('name') || $(this).attr('id');
const variableSource = $(this).attr('from');
$('variable,span').each((index, element) => {
const name = $(element).attr('name') || $(element).attr('id');
const variableSource = $(element).attr('from');

if (variableSource !== undefined) {
try {
const variableFilePath = path.resolve(userDefinedVariablesDir, variableSource);
const jsonData = fs.readFileSync(variableFilePath);
const varData = JSON.parse(jsonData);
Object.entries(varData).forEach(([varName, varValue]) => {
// Process the content of the variable with nunjucks, in case it refers to other variables.
const variableValue = njUtil.renderRaw(varValue, userDefinedVariables, {}, false);

userDefinedVariables[varName] = variableValue;
this.variablePreprocessor.renderAndAddUserDefinedVariable(base, varName, varValue);
});
} catch (err) {
logger.warn(`Error ${err.message}`);
}
} else {
// Process the content of the variable with nunjucks, in case it refers to other variables.
const html = njUtil.renderRaw($(this).html(), userDefinedVariables, {}, false);
userDefinedVariables[name] = html;
this.variablePreprocessor.renderAndAddUserDefinedVariable(base, name, $(element).html());
}
});
});
Expand Down Expand Up @@ -691,7 +688,6 @@ class Site {
const filePathArray = Array.isArray(filePaths) ? filePaths : [filePaths];
const uniquePaths = _.uniq(filePathArray);
logger.info('Rebuilding affected source files');
MarkBind.resetVariables();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we no longer need to reset the variables? What if a variable has been deleted? Would the old value associated with the variable also be cleared when rebuilding the page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, the process is made pure, while maintaining modest ~10% performance improvements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To add, the process before essentially involved 2x extraction and 2x rendering, storing the intermediate extracted variables in Parser's static variables.
The process is now pure since its just one step, so we have no need for that.

return new Promise((resolve, reject) => {
this.regenerateAffectedPages(uniquePaths)
.then(() => fs.removeAsync(this.tempPath))
Expand All @@ -709,7 +705,6 @@ class Site {
const uniqueUrls = _.uniq(normalizedUrlArray);
uniqueUrls.forEach(normalizedUrl => logger.info(
`Building ${normalizedUrl} as some of its dependencies were changed since the last visit`));
MarkBind.resetVariables();

/*
Lazy loading only builds the page being viewed, but the user may be quick enough
Expand All @@ -729,7 +724,6 @@ class Site {
}

this.toRebuild.delete(normalizedUrl);
pageToRebuild.userDefinedVariablesMap = this.userDefinedVariablesMap;
pageToRebuild.generate(new Set())
.then(() => {
pageToRebuild.collectHeadingsAndKeywords();
Expand Down Expand Up @@ -1062,7 +1056,6 @@ class Site {
}
}

page.userDefinedVariablesMap = this.userDefinedVariablesMap;
processingFiles.push(page.generate(builtFiles)
.catch((err) => {
logger.error(err);
Expand Down Expand Up @@ -1281,9 +1274,7 @@ class Site {
timeZoneName: 'short',
};
const time = new Date().toLocaleTimeString(this.siteConfig.locale, options);
Object.keys(this.userDefinedVariablesMap).forEach((base) => {
this.userDefinedVariablesMap[base].timestamp = time;
});
this.variablePreprocessor.addUserDefinedVariableForAllSites('timestamp', time);
return Promise.resolve();
}
}
Expand Down
5 changes: 0 additions & 5 deletions src/lib/markbind/src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,6 @@ module.exports = {

BOILERPLATE_FOLDER_NAME: '_markbind/boilerplates',

/* Imported global variables will be assigned a namespace.
* A prefix is appended to reduce clashes with other variables in the page.
*/
IMPORTED_VARIABLE_PREFIX: '$__MARKBIND__',

// src/lib/markbind/src/utils.js
markdownFileExts: ['md', 'mbd', 'mbdf'],
};
Loading