From caee33e08f0e4883825a8c63fa7bd35336e554ed Mon Sep 17 00:00:00 2001 From: Tan Wang Leng Date: Fri, 10 May 2019 20:23:17 +0800 Subject: [PATCH] Revert "Add imported variables (#751)" PR #751 implements imported variables, by deferring the rendering of Nunjucks variable to a later stage, so that the contents of the imported variables can be correctly populated before rendering takes place. However, this also means that we cannot do the following: {{ var }} Nunjucks rendering requires all variables to be resolved in one go. Since Nunjucks rendering is delayed in #751, the src attribute would not resolve correctly, so the logic for importing variables would fail and var would not have the correct content. However, if we do Nunjucks rendering at an earlier stage to resolve the src attribute, we will have to resolve the var variable immediately as well, as we have no way of selectively resolving certain variables at different stages. This is because we do not have an algorithm to determine which variables should be resolved and which shouldn't be resolved (this said algorithm must also take into consider edge cases, such as using Nunjucks' "set" instruction). Therefore, the design of import variable needs to be reconsidered in order to overcome this techincal obstacle. Right now, the failure to resolve the src attribute properly, due to the deferred Nunjuck rendering, is breaking website generation (for example, MarkBind's "userGuide/formattingContents.html" is broken). There doesn't seem to be a quick fix to overcome the technical obstacle described above. The new design might take additional time, and authors also have no quick ways to solve this issue. Let's revert the import variable PR (#751) to prevent generated websites from breaking, and wait for a new implementation of import variables to surface instead. This reverts commit 4412153abd42b1b99c550bc5be195c99168592ac. --- src/lib/markbind/src/parser.js | 133 ++++-------------- test/functional/test_site/expected/index.html | 14 -- .../test_site/expected/siteData.json | 3 - test/functional/test_site/index.md | 6 - .../test_site/testImportedVariables.md | 7 - .../testImportedVariablesToImport.md | 8 -- 6 files changed, 29 insertions(+), 142 deletions(-) delete mode 100644 test/functional/test_site/testImportedVariables.md delete mode 100644 test/functional/test_site/testImportedVariablesToImport.md diff --git a/src/lib/markbind/src/parser.js b/src/lib/markbind/src/parser.js index 95eaf6c683..cb09328ed7 100644 --- a/src/lib/markbind/src/parser.js +++ b/src/lib/markbind/src/parser.js @@ -27,8 +27,6 @@ const ATTRIB_CWF = 'cwf'; const BOILERPLATE_FOLDER_NAME = '_markbind/boilerplates'; -const VARIABLE_LOOKUP = {}; - /* * Utils */ @@ -144,47 +142,14 @@ function extractPageVariables(fileName, data, userDefinedVariables, includedVari return; } if (!pageVariables[variableName]) { - const variableValue + pageVariables[variableName] = nunjucks.renderString(md.renderInline(variableElement.html()), { ...pageVariables, ...userDefinedVariables, ...includedVariables }); - pageVariables[variableName] = variableValue; - if (!VARIABLE_LOOKUP[fileName]) { - VARIABLE_LOOKUP[fileName] = {}; - } - VARIABLE_LOOKUP[fileName][variableName] = variableValue; } }); return pageVariables; } -/** - * Extract imported page variables from a page - * @param context of the page - */ -function extractImportedVariables(context) { - if (!context.importedVariables) { - return {}; - } - const importedVariables = {}; - Object.entries(context.importedVariables).forEach(([src, variables]) => { - variables.forEach((variableName) => { - const actualFilePath = utils.isUrl() - ? src - : path.resolve(path.dirname(context.cwf), decodeURIComponent(url.parse(src).path)); - if (!VARIABLE_LOOKUP[actualFilePath] || !VARIABLE_LOOKUP[actualFilePath][variableName]) { - // eslint-disable-next-line no-console - console.warn(`Missing variable ${variableName} in ${src} referenced by ${context.cwf}\n`); - return; - } - const variableValue = VARIABLE_LOOKUP[actualFilePath][variableName]; - if (!importedVariables[variableName]) { - importedVariables[variableName] = variableValue; - } - }); - }); - return importedVariables; -} - Parser.prototype.getDynamicIncludeSrc = function () { return _.clone(this.dynamicIncludeSrc); }; @@ -207,13 +172,13 @@ Parser.prototype._preprocess = function (node, context, config) { element.attribs = element.attribs || {}; element.attribs[ATTRIB_CWF] = path.resolve(context.cwf); - const requiresSrc = ['include', 'import'].includes(element.name); + const requiresSrc = ['include'].includes(element.name); if (requiresSrc && _.isEmpty(element.attribs.src)) { const error = new Error(`Empty src attribute in ${element.name} in: ${element.attribs[ATTRIB_CWF]}`); this._onError(error); return createErrorNode(element, error); } - const shouldProcessSrc = ['include', 'panel', 'import'].includes(element.name); + const shouldProcessSrc = ['include', 'panel'].includes(element.name); const hasSrc = _.hasIn(element.attribs, 'src'); let isUrl; let includeSrc; @@ -246,8 +211,7 @@ Parser.prototype._preprocess = function (node, context, config) { } } - if (element.name === 'include' || element.name === 'import') { - const isImport = element.name === 'import'; + if (element.name === 'include') { const isInline = _.hasIn(element.attribs, 'inline'); const isDynamic = _.hasIn(element.attribs, 'dynamic'); const isOptional = _.hasIn(element.attribs, 'optional'); @@ -299,7 +263,7 @@ Parser.prototype._preprocess = function (node, context, config) { element.name = 'markdown'; } - const fileContent = self._fileCache[actualFilePath]; // cache the file contents to save some I/O + let fileContent = self._fileCache[actualFilePath]; // cache the file contents to save some I/O const { parent, relative } = calculateNewBaseUrls(filePath, config.rootPath, config.baseUrlMap); const userDefinedVariables = config.userDefinedVariablesMap[path.resolve(parent, relative)]; @@ -307,9 +271,14 @@ Parser.prototype._preprocess = function (node, context, config) { const includeVariables = extractIncludeVariables(element, context.variables); // Extract page variables from the CHILD file - const pageVariables = extractPageVariables(actualFilePath, fileContent, + const pageVariables = extractPageVariables(element.attribs.src, fileContent, userDefinedVariables, includeVariables); + // Render inner file content + fileContent = nunjucks.renderString(fileContent, + { ...pageVariables, ...includeVariables, ...userDefinedVariables }, + { path: actualFilePath }); + // Delete variable attributes in include Object.keys(element.attribs).forEach((attribute) => { if (attribute.startsWith('var-')) { @@ -327,28 +296,8 @@ Parser.prototype._preprocess = function (node, context, config) { const segmentSrc = cheerio.parseHTML(fileContent, true); const $ = cheerio.load(segmentSrc); const hashContent = $(includeSrc.hash).html(); - - if (isImport) { - const variableContent = $(`variable[name=${includeSrc.hash.substring(1)}]`).html(); - if (!variableContent) { - // eslint-disable-next-line no-console - console.warn(`Missing import variable ${includeSrc.pathname} in ${element.attribs[ATTRIB_CWF]}.`); - return createEmptyNode(); - } - if (!context.importedVariables) { - // eslint-disable-next-line no-param-reassign - context.importedVariables = {}; - } - if (!context.importedVariables[includeSrc.pathname]) { - // eslint-disable-next-line no-param-reassign - context.importedVariables[includeSrc.pathname] = []; - } - // eslint-disable-next-line no-param-reassign - context.importedVariables[includeSrc.pathname].push(includeSrc.hash.substring(1)); - return createEmptyNode(); - } - let actualContent = (hashContent && isTrim) ? hashContent.trim() : hashContent; + if (actualContent === null) { if (isOptional) { // set empty content for optional segment include that does not exist @@ -382,12 +331,6 @@ Parser.prototype._preprocess = function (node, context, config) { true, ); } else { - if (isImport) { - // eslint-disable-next-line no-console - console.warn(`Missing hash for import variable ${includeSrc.pathname}` - + ` in ${element.attribs[ATTRIB_CWF]}.`); - return createEmptyNode(); - } let actualContent = (fileContent && isTrim) ? fileContent.trim() : fileContent; if (isIncludeSrcMd) { if (context.mode === 'include') { @@ -409,8 +352,7 @@ Parser.prototype._preprocess = function (node, context, config) { childContext.cwf = filePath; childContext.source = isIncludeSrcMd ? 'md' : 'html'; childContext.callStack.push(context.cwf); - childContext.variables = { ...includeVariables }; - childContext.importedVariables = {}; + childContext.variables = includeVariables; if (element.children && element.children.length > 0) { if (childContext.callStack.length > CyclicReferenceError.MAX_RECURSIVE_DEPTH) { @@ -418,21 +360,7 @@ Parser.prototype._preprocess = function (node, context, config) { this._onError(error); return createErrorNode(element, error); } - element.children = element.children.map((e) => { - let processedEle = cheerio.html(self._preprocess(e, childContext, config)); - const importedVariables = extractImportedVariables(childContext); - userDefinedVariables.hostBaseUrl = '{{hostBaseUrl}}'; - processedEle - = nunjucks.renderString(processedEle, - { - ...pageVariables, - ...importedVariables, - ...includeVariables, - ...userDefinedVariables, - }, - { path: actualFilePath }); - return cheerio.parseHTML(processedEle)[0]; - }); + element.children = element.children.map(e => self._preprocess(e, childContext, config)); } } else if ((element.name === 'panel') && hasSrc) { if (!isUrl && includeSrc.hash) { @@ -603,17 +531,8 @@ Parser.prototype.includeFile = function (file, config) { context.cwf = config.cwf || file; // current working file context.mode = 'include'; context.callStack = []; - let fileData = null; return new Promise((resolve, reject) => { - let actualFilePath = file; - if (!utils.fileExists(file)) { - const boilerplateFilePath = calculateBoilerplateFilePath(path.basename(file), file, config); - if (utils.fileExists(boilerplateFilePath)) { - actualFilePath = boilerplateFilePath; - } - } - const handler = new htmlparser.DomHandler((error, dom) => { if (error) { reject(error); @@ -630,13 +549,7 @@ Parser.prototype.includeFile = function (file, config) { } return processed; }); - const { parent, relative } = calculateNewBaseUrls(file, config.rootPath, config.baseUrlMap); - const userDefinedVariables = config.userDefinedVariablesMap[path.resolve(parent, relative)]; - const pageVariables = extractPageVariables(path.basename(file), fileData, userDefinedVariables, {}); - const importedVariables = extractImportedVariables(context); - resolve(nunjucks.renderString(cheerio.html(nodes), - { ...pageVariables, ...importedVariables, ...userDefinedVariables }, - { path: actualFilePath })); + resolve(cheerio.html(nodes)); }); const parser = new htmlparser.Parser(handler, { @@ -644,15 +557,27 @@ Parser.prototype.includeFile = function (file, config) { decodeEntities: true, }); + let actualFilePath = file; + if (!utils.fileExists(file)) { + const boilerplateFilePath = calculateBoilerplateFilePath(path.basename(file), file, config); + if (utils.fileExists(boilerplateFilePath)) { + actualFilePath = boilerplateFilePath; + } + } + // Read files fs.readFile(actualFilePath, 'utf-8', (err, data) => { - fileData = data; if (err) { reject(err); return; } + 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 }, + { path: actualFilePath }); const fileExt = utils.getExt(file); - const fileContent = data; if (utils.isMarkdownFileExt(fileExt)) { context.source = 'md'; parser.parseComplete(fileContent.toString()); diff --git a/test/functional/test_site/expected/index.html b/test/functional/test_site/expected/index.html index 8cefa5e59a..960adbde67 100644 --- a/test/functional/test_site/expected/index.html +++ b/test/functional/test_site/expected/index.html @@ -183,17 +183,6 @@

Page Variable Referencing I
Page Variable Referencing Included Variable -
-

Imported Variable

-
- Imported Variable -

Imported Variable Referencing Global Variable

-
- Imported Variable Referencing Global Variable -
-

Imported Variable on Top Level

-
- Imported Variable on Top Level

Heading with multiple keywords

keyword 1 keyword 2

@@ -486,9 +475,6 @@
Outer Page Variable Should Not Leak Into Inner Pages‎ Included Variable Overriding Page Variable‎ Page Variable Referencing Included Variable‎ - Imported Variable‎ - Imported Variable Referencing Global Variable‎ - Imported Variable on Top Level‎ Heading with multiple keywords‎ Heading with keyword in panel‎ Panel with heading with keyword‎ diff --git a/test/functional/test_site/expected/siteData.json b/test/functional/test_site/expected/siteData.json index 5e62355620..a290bcd6a8 100644 --- a/test/functional/test_site/expected/siteData.json +++ b/test/functional/test_site/expected/siteData.json @@ -53,9 +53,6 @@ "outer-page-variable-should-not-leak-into-inner-pages": "Outer Page Variable Should Not Leak Into Inner Pages", "included-variable-overriding-page-variable": "Included Variable Overriding Page Variable", "page-variable-referencing-included-variable": "Page Variable Referencing Included Variable", - "imported-variable": "Imported Variable", - "imported-variable-referencing-global-variable": "Imported Variable Referencing Global Variable", - "imported-variable-on-top-level": "Imported Variable on Top Level", "heading-with-multiple-keywords": "Heading with multiple keywords | keyword 1, keyword 2", "heading-with-keyword-in-panel": "Heading with keyword in panel | panel keyword", "expanded-panel-without-heading-with-keyword": "Expanded panel without heading with keyword", diff --git a/test/functional/test_site/index.md b/test/functional/test_site/index.md index 68c47c1de8..18d75bf8b3 100644 --- a/test/functional/test_site/index.md +++ b/test/functional/test_site/index.md @@ -55,12 +55,6 @@ tags: ["tag-frontmatter-shown", "tag-included-file", "+tag-exp*", "-tag-exp-hidd Included Variable Overriding Page Variable - - -# Imported Variable on Top Level - -{{ imported_variable_on_top_level }} - # Heading with multiple keywords keyword 1 keyword 2 diff --git a/test/functional/test_site/testImportedVariables.md b/test/functional/test_site/testImportedVariables.md deleted file mode 100644 index 78f9409e08..0000000000 --- a/test/functional/test_site/testImportedVariables.md +++ /dev/null @@ -1,7 +0,0 @@ -# Imported Variable - -{{ imported_variable }} - -# Imported Variable Referencing Global Variable - -{{ imported_variable_referencing_global_variable }} diff --git a/test/functional/test_site/testImportedVariablesToImport.md b/test/functional/test_site/testImportedVariablesToImport.md deleted file mode 100644 index 84d431bdec..0000000000 --- a/test/functional/test_site/testImportedVariablesToImport.md +++ /dev/null @@ -1,8 +0,0 @@ -# **Should not appear**: Imported Variable -Imported Variable - -# **Should not appear**: Imported Variable on Top Level -Imported Variable on Top Level - -# **Should not appear**: Imported Variable Referencing Global Variable -Imported Variable Referencing {{ global_variable }}