From cec28a47c1b0640b65231cb58f27e9a512f72636 Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Wed, 9 Jun 2021 16:57:00 -0400 Subject: [PATCH 01/17] add short names to all-versions --- lib/all-versions.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/all-versions.js b/lib/all-versions.js index 64fe6c9c8d..745fa45b10 100644 --- a/lib/all-versions.js +++ b/lib/all-versions.js @@ -11,6 +11,7 @@ const plans = [ // See lib/remove-fpt-from-path.js for details. plan: 'free-pro-team', planTitle: 'GitHub.com', + shortName: 'fpt', releases: [latestNonNumberedRelease], latestRelease: latestNonNumberedRelease, nonEnterpriseDefault: true, // permanent way to refer to this plan if the name changes @@ -20,6 +21,7 @@ const plans = [ { plan: 'enterprise-server', planTitle: 'Enterprise Server', + shortName: 'ghes', releases: enterpriseServerReleases.supported, latestRelease: enterpriseServerReleases.latest, hasNumberedReleases: true, @@ -29,6 +31,7 @@ const plans = [ { plan: 'github-ae', planTitle: 'GitHub AE', + shortName: 'ghae', releases: [latestNonNumberedRelease], latestRelease: latestNonNumberedRelease, openApiBaseName: 'github.ae', From 1dfd1917ca74e4aae7adbfb58e9346956a0a31ce Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Wed, 9 Jun 2021 16:57:31 -0400 Subject: [PATCH 02/17] register new ifver Liquid tag --- lib/liquid-tags/ifver.js | 107 ++++++++++++++++++++++++++++++++++++ lib/render-content/index.js | 1 + 2 files changed, 108 insertions(+) create mode 100644 lib/liquid-tags/ifver.js diff --git a/lib/liquid-tags/ifver.js b/lib/liquid-tags/ifver.js new file mode 100644 index 0000000000..143fd142a4 --- /dev/null +++ b/lib/liquid-tags/ifver.js @@ -0,0 +1,107 @@ +const { isTruthy, Expression, TokenizationError } = require('liquidjs') +const versionSatisfiesRange = require('../version-satisfies-range') + +const SyntaxHelp = "Syntax Error in 'ifver' with range - Valid syntax: ifver [operator] [releaseNumber]" + +const supportedOperators = [ + '=', + '<', + '>' +] + +const supportedOperatorsRegex = new RegExp(`[${supportedOperators.join('')}]`) +const releaseRegex = /\d\d?\.\d\d?/ + +// This module supports a new tag we can use for docs versioning specifically. It extends the +// native Liquid `if` block tag. It has special handling for statements like {% ifver ghes < 3.0 %}, +// using semver to evaluate release numbers instead of doing standard number comparisons, which +// don't work the way we want because they evaluate 3.2 < 3.10 = true. +module.exports = { + parse (tagToken, remainTokens) { + this.tagToken = tagToken + this.branches = [] + this.elseTemplates = [] + + let p + const stream = this.liquid.parser.parseStream(remainTokens) + .on('start', () => this.branches.push({ + cond: tagToken.args, + templates: (p = []) + })) + .on('tag:elsif', (token) => { + this.branches.push({ + cond: token.args, + templates: p = [] + }) + }) + .on('tag:else', () => (p = this.elseTemplates)) + .on('tag:endif', () => stream.stop()) + .on('template', (tpl) => p.push(tpl)) + .on('end', () => { + throw new Error(`tag ${tagToken.getText()} not closed`) + }) + + stream.start() + }, + + render: function * (ctx, emitter) { + const r = this.liquid.renderer + const { operators, operatorsTrie } = this.liquid.options + + for (const branch of this.branches) { + // Resolve special operators in the conditional, if any. + // This will replace syntax like `fpt or ghes < 3.0` with `fpt or true` or `fpt or false`. + const resolvedBranchCond = this.handleOperators(branch.cond, ctx.environments) + + // Use Liquid's native function for the final evaluation. + const cond = yield new Expression(resolvedBranchCond, operators, operatorsTrie, ctx.opts.lenientIf).value(ctx) + + if (isTruthy(cond, ctx)) { + yield r.renderTemplates(branch.templates, ctx, emitter) + return + } + } + yield r.renderTemplates(this.elseTemplates, ctx, emitter) + }, + + handleOperators (resolvedBranchCond, context) { + if (!supportedOperatorsRegex.test(resolvedBranchCond)) return resolvedBranchCond + + // If this conditional contains multiple parts using `or` or `and`, get only the conditional with operators. + const condArray = resolvedBranchCond.split(' ') + + // Find the first index in the array that contains an operator. + const operatorIndex = condArray.findIndex(el => supportedOperators.find(op => el === op)) + + // E.g., ['ghae', '<', '3.1'] + const condParts = condArray.slice(operatorIndex - 1, operatorIndex + 2) + + // Assign to vars. + const [versionShortName, operator, releaseToEvaluate] = condParts + + // Handle syntax errors. + const error = !supportedOperators.includes(operator) || !releaseRegex.test(releaseToEvaluate) + + if (error) { + throw new TokenizationError(SyntaxHelp, this.tagToken) + } + + const currentRelease = context.currentVersion.split('@')[1] + const currentVersionShortName = context.allVersions[context.currentVersion].shortName + + // Evaluate the operator if this is the current version; otherwise it's always false. + const resolvedBoolean = versionShortName === currentVersionShortName + ? versionSatisfiesRange(currentRelease, `${operator}${releaseToEvaluate}`) + : false + + // Replace syntax like `fpt or ghes < 3.0` with `fpt or true` or `fpt or false`. + resolvedBranchCond = resolvedBranchCond.replace(condParts.join(' '), resolvedBoolean) + + // Run this function recursively until we've resolved all the special operators. + if (supportedOperatorsRegex.test(resolvedBranchCond)) { + return this.handleOperators(resolvedBranchCond, context) + } + + return resolvedBranchCond + } +} diff --git a/lib/render-content/index.js b/lib/render-content/index.js index 9b58102ad8..689e110540 100644 --- a/lib/render-content/index.js +++ b/lib/render-content/index.js @@ -12,6 +12,7 @@ renderContent.liquid.registerTag('indented_data_reference', require('../liquid-t renderContent.liquid.registerTag('data', require('../liquid-tags/data')) renderContent.liquid.registerTag('octicon', require('../liquid-tags/octicon')) renderContent.liquid.registerTag('link_as_article_card', require('../liquid-tags/link-as-article-card')) +renderContent.liquid.registerTag('ifver', require('../liquid-tags/ifver')) for (const tag in tags) { // Register all the extended markdown tags, like {% note %} and {% warning %} From 49a64c2227259119fc9a9ae08eafcf555efa16fd Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Wed, 9 Jun 2021 16:57:51 -0400 Subject: [PATCH 03/17] add clarifying comments --- lib/version-satisfies-range.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/version-satisfies-range.js b/lib/version-satisfies-range.js index d5e2c9be0a..21f7a27066 100644 --- a/lib/version-satisfies-range.js +++ b/lib/version-satisfies-range.js @@ -1,8 +1,10 @@ const semver = require('semver') -// workaround for Enterprise Server 11.10.340 because we can't use semver to -// compare it to 2.x like we can with 2.0+ +// Where "version" is an Enterprise Server release number, like `3.1`, +// and "range" is a semver range operator with another number, like `<=3.2`. module.exports = function versionSatisfiesRange (version, range) { + // workaround for Enterprise Server 11.10.340 because we can't use semver to + // compare it to 2.x like we can with 2.0+ if (version === '11.10.340') return range.startsWith('<') return semver.satisfies(semver.coerce(version), range) From d1cf56abd593055725396e9f7f2296a9bee45c5e Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Wed, 9 Jun 2021 16:58:19 -0400 Subject: [PATCH 04/17] add short names = true to context object so we can access them in Liquid conditionals --- middleware/contextualizers/short-versions.js | 20 ++++++++++++++++++++ middleware/index.js | 1 + 2 files changed, 21 insertions(+) create mode 100644 middleware/contextualizers/short-versions.js diff --git a/middleware/contextualizers/short-versions.js b/middleware/contextualizers/short-versions.js new file mode 100644 index 0000000000..0957558a08 --- /dev/null +++ b/middleware/contextualizers/short-versions.js @@ -0,0 +1,20 @@ +// This module creates shortcuts for version comparisons in Liquid conditional strings. +// +// Supported: +// {% if fpt %} +// {% if ghae %} +// {% if ghes %} +// +// For the custom operator handling in statements like {% if ghes > 3.0 %}, see `lib/liquid-tags/if-ver.js`. +module.exports = async function shortVersions (req, res, next) { + if (!req.context.page) return next() + + const { allVersions, currentVersion } = req.context + const currentVersionObj = allVersions[currentVersion] + if (!currentVersionObj) return next() + + // Add the short name to context. + req.context[currentVersionObj.shortName] = true + + return next() +} diff --git a/middleware/index.js b/middleware/index.js index 7cffb68e5d..14547e169a 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -60,6 +60,7 @@ module.exports = function (app) { app.use(require('./record-redirect')) app.use(instrument('./detect-language')) // Must come before context, breadcrumbs, find-page, handle-errors, homepages app.use(asyncMiddleware(instrument('./context'))) // Must come before early-access-*, handle-redirects + app.use(asyncMiddleware(instrument('./contextualizers/short-versions'))) // Support version shorthands // *** Redirects, 3xx responses *** // I ordered these by use frequency From 7a93a3a639619f164c04847701d1eaa09e51a186 Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Wed, 9 Jun 2021 16:58:39 -0400 Subject: [PATCH 05/17] add tests for the ifver tag --- tests/unit/liquid.js | 92 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 75 insertions(+), 17 deletions(-) diff --git a/tests/unit/liquid.js b/tests/unit/liquid.js index 4fbe06059a..b852c161de 100644 --- a/tests/unit/liquid.js +++ b/tests/unit/liquid.js @@ -1,28 +1,36 @@ const { liquid } = require('../../lib/render-content') +const middleware = require('../../middleware/contextualizers/short-versions') +const allVersions = require('../../lib/all-versions') +const enterpriseServerReleases = require('../../lib/enterprise-server-releases') + const template = ` - {% if page.version ver_gt "2.13" %}up to date{% endif %} - {% if page.version ver_lt "2.13" %}out of date{% endif %} + {% if currentVersion ver_gt "enterprise-server@2.13" %}up to date{% endif %} + {% if currentVersion ver_lt "enterprise-server@2.13" %}out of date{% endif %} +` + +const shortVersionsTemplate = ` + {% ifver fpt %} I am FPT {% endif %} + {% ifver ghae %} I am GHAE {% endif %} + {% ifver ghes %} I am GHES {% endif %} + {% ifver ghes = 3.1 %} I am GHES = 3.1 {% endif %} + {% ifver ghes > 3.1 %} I am GHES > 3.1 {% endif %} + {% ifver ghes < 3.1 %} I am GHES < 3.1 {% endif %} + {% ifver fpt or ghes < 3.0 %} I am FTP or GHES < 3.0 {% endif %} + {% ifver ghes < 3.1 and ghes > 2.22 %} I am 3.0 only {% endif %} ` describe('liquid template parser', () => { describe('custom operators', () => { describe('ver_gt', () => { test('works as expected', async () => { - const context = { - page: { - version: '2.14' - } - } + const context = { currentVersion: 'enterprise-server@2.14' } const output = await liquid.parseAndRender(template, context) expect(output.trim()).toBe('up to date') }) test('returns false when given value is not numeric, like `dotcom`', async () => { - const context = { - page: { - version: 'dotcom' - } - } + const context = { currentVersion: 'free-pro-team@latest' } + const output = await liquid.parseAndRender(template, context) expect(output.trim()).toBe('') }) @@ -36,14 +44,64 @@ describe('liquid template parser', () => { describe('ver_lt', () => { test('works as expected', async () => { - const context = { - page: { - version: '2.12' - } - } + const context = { currentVersion: 'enterprise-server@2.12' } const output = await liquid.parseAndRender(template, context) expect(output.trim()).toBe('out of date') }) }) }) + + describe('short versions', () => { + // Create a fake req so we can test the shortVersions middleware + const req = { language: 'en', query: {} } + + test('FPT works as expected when it is FPT', async () => { + req.context = { + currentVersion: 'free-pro-team@latest', + page: {}, + allVersions, + enterpriseServerReleases + } + await middleware(req, null, () => {}) + const output = await liquid.parseAndRender(shortVersionsTemplate, req.context) + // We should have TWO results because we are supporting two shortcuts + expect(output.replace(/\s\s+/g, ' ').trim()).toBe('I am FPT I am FTP or GHES < 3.0') + }) + + test('GHAE works as expected', async () => { + req.context = { + currentVersion: 'github-ae@latest', + page: {}, + allVersions, + enterpriseServerReleases + } + await middleware(req, null, () => {}) + const output = await liquid.parseAndRender(shortVersionsTemplate, req.context) + expect(output.trim()).toBe('I am GHAE') + }) + + test('GHES works as expected', async () => { + req.context = { + currentVersion: 'enterprise-server@2.22', + page: {}, + allVersions, + enterpriseServerReleases + } + await middleware(req, null, () => {}) + const output = await liquid.parseAndRender(shortVersionsTemplate, req.context) + expect(output.replace(/\s\s+/g, ' ').trim()).toBe('I am GHES I am GHES < 3.1 I am FTP or GHES < 3.0') + }) + + test('AND statements work as expected', async () => { + req.context = { + currentVersion: 'enterprise-server@3.0', + page: {}, + allVersions, + enterpriseServerReleases + } + await middleware(req, null, () => {}) + const output = await liquid.parseAndRender(shortVersionsTemplate, req.context) + expect(output.replace(/\s\s+/g, ' ').trim()).toBe('I am GHES I am GHES < 3.1 I am 3.0 only') + }) + }) }) From ca30a48ba6e64abf6fea51703056ae156c1c5033 Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Wed, 9 Jun 2021 16:59:09 -0400 Subject: [PATCH 06/17] add shortName as a required prop in the versions schema --- tests/helpers/schemas/versions-schema.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/helpers/schemas/versions-schema.js b/tests/helpers/schemas/versions-schema.js index dff8c8182b..7781fbbacd 100644 --- a/tests/helpers/schemas/versions-schema.js +++ b/tests/helpers/schemas/versions-schema.js @@ -6,6 +6,7 @@ const delimiter = '@' const versionPattern = `${planPattern}${delimiter}${releasePattern}` module.exports = { + additionalProperties: false, properties: { version: { required: true, @@ -40,6 +41,11 @@ module.exports = { description: 'the plan title', // this is the same as the version title, sans numbered release type: 'string' }, + shortName: { + required: true, + description: 'the short name for the version to be used in Liquid conditionals', + type: 'string' + }, releases: { required: true, description: 'an array of all supported releases for the version', From 4f43873bbcd5da6ae47730a40e277ae1ccbb5ec4 Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Wed, 9 Jun 2021 17:00:05 -0400 Subject: [PATCH 07/17] throw errors if we are trying to evaluate strings in Liquid conditionals (they always evaluate to true and may render content we don't want) --- tests/linting/lint-files.js | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/linting/lint-files.js b/tests/linting/lint-files.js index 5fcc88f212..61be4d54f1 100644 --- a/tests/linting/lint-files.js +++ b/tests/linting/lint-files.js @@ -146,6 +146,11 @@ const oldOcticonRegex = /{{\s*?octicon-([a-z-]+)(\s[\w\s\d-]+)?\s*?}}/g // const oldExtendedMarkdownRegex = /{{\s*?[#/][a-z-]+\s*?}}/g +// Strings in Liquid will always evaluate true _because_ they are strings; instead use unquoted variables, like {% if foo %}. +// - {% if "foo" %} +// - {% unless "bar" %} +const stringInLiquidRegex = /{% (?:if|ifver|elseif|unless) (?:"|').+?%}/g + const relativeArticleLinkErrorText = 'Found unexpected relative article links:' const languageLinkErrorText = 'Found article links with hard-coded language codes:' const versionLinkErrorText = 'Found article links with hard-coded version numbers:' @@ -156,6 +161,7 @@ const badEarlyAccessImageErrorText = 'Found article images/links leaking incorre const oldVariableErrorText = 'Found article uses old {{ site.data... }} syntax. Use {% data example.data.string %} instead!' const oldOcticonErrorText = 'Found octicon variables with the old {{ octicon-name }} syntax. Use {% octicon "name" %} instead!' const oldExtendedMarkdownErrorText = 'Found extended markdown tags with the old {{#note}} syntax. Use {% note %}/{% endnote %} instead!' +const stringInLiquidErrorText = 'Found Liquid conditionals that evaluate a string instead of a variable. Remove the quotes around the variable!' const mdWalkOptions = { globs: ['**/*.md'], @@ -407,6 +413,12 @@ describe('lint markdown content', () => { }) }) + test('does not contain Liquid that evaluates strings (because they are always true)', async () => { + const matches = (content.match(stringInLiquidRegex) || []) + const errorMessage = formatLinkError(stringInLiquidErrorText, matches) + expect(matches.length, errorMessage).toBe(0) + }) + test('URLs must not contain a hard-coded language code', async () => { const matches = links.filter(link => { return /\/(?:${languageCodes.join('|')})\//.test(link) @@ -666,6 +678,22 @@ describe('lint yaml content', () => { const errorMessage = formatLinkError(oldExtendedMarkdownErrorText, matches) expect(matches.length, errorMessage).toBe(0) }) + + test('does not contain Liquid that evaluates strings (because they are always true)', async () => { + const matches = [] + + for (const [key, content] of Object.entries(dictionary)) { + const contentStr = getContent(content) + if (!contentStr) continue + const valMatches = (contentStr.match(stringInLiquidRegex) || []) + if (valMatches.length > 0) { + matches.push(...valMatches.map((match) => `Key "${key}": ${match}`)) + } + } + + const errorMessage = formatLinkError(stringInLiquidErrorText, matches) + expect(matches.length, errorMessage).toBe(0) + }) } } ) From 6b2df4de541826f9b537e36519ce3e58ba7d4a00 Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Wed, 9 Jun 2021 17:12:14 -0400 Subject: [PATCH 08/17] Update lib/liquid-tags/ifver.js --- lib/liquid-tags/ifver.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/liquid-tags/ifver.js b/lib/liquid-tags/ifver.js index 143fd142a4..c844f7ae1f 100644 --- a/lib/liquid-tags/ifver.js +++ b/lib/liquid-tags/ifver.js @@ -15,7 +15,7 @@ const releaseRegex = /\d\d?\.\d\d?/ // This module supports a new tag we can use for docs versioning specifically. It extends the // native Liquid `if` block tag. It has special handling for statements like {% ifver ghes < 3.0 %}, // using semver to evaluate release numbers instead of doing standard number comparisons, which -// don't work the way we want because they evaluate 3.2 < 3.10 = true. +// don't work the way we want because they evaluate 3.2 > 3.10 = true. module.exports = { parse (tagToken, remainTokens) { this.tagToken = tagToken From 534996d81c07afee764cfc10481f105983c2f04a Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Thu, 10 Jun 2021 11:53:56 -0400 Subject: [PATCH 09/17] change ifver to ifversion --- lib/liquid-tags/{ifver.js => ifversion.js} | 4 ++-- lib/render-content/index.js | 2 +- tests/linting/lint-files.js | 2 +- tests/unit/liquid.js | 16 ++++++++-------- 4 files changed, 12 insertions(+), 12 deletions(-) rename lib/liquid-tags/{ifver.js => ifversion.js} (96%) diff --git a/lib/liquid-tags/ifver.js b/lib/liquid-tags/ifversion.js similarity index 96% rename from lib/liquid-tags/ifver.js rename to lib/liquid-tags/ifversion.js index 143fd142a4..1ddb524875 100644 --- a/lib/liquid-tags/ifver.js +++ b/lib/liquid-tags/ifversion.js @@ -1,7 +1,7 @@ const { isTruthy, Expression, TokenizationError } = require('liquidjs') const versionSatisfiesRange = require('../version-satisfies-range') -const SyntaxHelp = "Syntax Error in 'ifver' with range - Valid syntax: ifver [operator] [releaseNumber]" +const SyntaxHelp = "Syntax Error in 'ifversion' with range - Valid syntax: ifversion [operator] [releaseNumber]" const supportedOperators = [ '=', @@ -13,7 +13,7 @@ const supportedOperatorsRegex = new RegExp(`[${supportedOperators.join('')}]`) const releaseRegex = /\d\d?\.\d\d?/ // This module supports a new tag we can use for docs versioning specifically. It extends the -// native Liquid `if` block tag. It has special handling for statements like {% ifver ghes < 3.0 %}, +// native Liquid `if` block tag. It has special handling for statements like {% ifversion ghes < 3.0 %}, // using semver to evaluate release numbers instead of doing standard number comparisons, which // don't work the way we want because they evaluate 3.2 < 3.10 = true. module.exports = { diff --git a/lib/render-content/index.js b/lib/render-content/index.js index 689e110540..8186a63c56 100644 --- a/lib/render-content/index.js +++ b/lib/render-content/index.js @@ -12,7 +12,7 @@ renderContent.liquid.registerTag('indented_data_reference', require('../liquid-t renderContent.liquid.registerTag('data', require('../liquid-tags/data')) renderContent.liquid.registerTag('octicon', require('../liquid-tags/octicon')) renderContent.liquid.registerTag('link_as_article_card', require('../liquid-tags/link-as-article-card')) -renderContent.liquid.registerTag('ifver', require('../liquid-tags/ifver')) +renderContent.liquid.registerTag('ifversion', require('../liquid-tags/ifversion')) for (const tag in tags) { // Register all the extended markdown tags, like {% note %} and {% warning %} diff --git a/tests/linting/lint-files.js b/tests/linting/lint-files.js index 8cda33982f..14cfd1c024 100644 --- a/tests/linting/lint-files.js +++ b/tests/linting/lint-files.js @@ -149,7 +149,7 @@ const oldExtendedMarkdownRegex = /{{\s*?[#/][a-z-]+\s*?}}/g // Strings in Liquid will always evaluate true _because_ they are strings; instead use unquoted variables, like {% if foo %}. // - {% if "foo" %} // - {% unless "bar" %} -const stringInLiquidRegex = /{% (?:if|ifver|elseif|unless) (?:"|').+?%}/g +const stringInLiquidRegex = /{% (?:if|ifversion|elseif|unless) (?:"|').+?%}/g const relativeArticleLinkErrorText = 'Found unexpected relative article links:' const languageLinkErrorText = 'Found article links with hard-coded language codes:' diff --git a/tests/unit/liquid.js b/tests/unit/liquid.js index b852c161de..33323d278a 100644 --- a/tests/unit/liquid.js +++ b/tests/unit/liquid.js @@ -9,14 +9,14 @@ const template = ` ` const shortVersionsTemplate = ` - {% ifver fpt %} I am FPT {% endif %} - {% ifver ghae %} I am GHAE {% endif %} - {% ifver ghes %} I am GHES {% endif %} - {% ifver ghes = 3.1 %} I am GHES = 3.1 {% endif %} - {% ifver ghes > 3.1 %} I am GHES > 3.1 {% endif %} - {% ifver ghes < 3.1 %} I am GHES < 3.1 {% endif %} - {% ifver fpt or ghes < 3.0 %} I am FTP or GHES < 3.0 {% endif %} - {% ifver ghes < 3.1 and ghes > 2.22 %} I am 3.0 only {% endif %} + {% ifversion fpt %} I am FPT {% endif %} + {% ifversion ghae %} I am GHAE {% endif %} + {% ifversion ghes %} I am GHES {% endif %} + {% ifversion ghes = 3.1 %} I am GHES = 3.1 {% endif %} + {% ifversion ghes > 3.1 %} I am GHES > 3.1 {% endif %} + {% ifversion ghes < 3.1 %} I am GHES < 3.1 {% endif %} + {% ifversion fpt or ghes < 3.0 %} I am FTP or GHES < 3.0 {% endif %} + {% ifversion ghes < 3.1 and ghes > 2.22 %} I am 3.0 only {% endif %} ` describe('liquid template parser', () => { From 33cec6f10fc0487d75ec12130a6222635ae7d13f Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Thu, 10 Jun 2021 12:12:00 -0400 Subject: [PATCH 10/17] add linter to catch use of {% if version %} instead of {% ifversion %} --- tests/linting/lint-files.js | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/linting/lint-files.js b/tests/linting/lint-files.js index 14cfd1c024..b367995fdd 100644 --- a/tests/linting/lint-files.js +++ b/tests/linting/lint-files.js @@ -151,6 +151,9 @@ const oldExtendedMarkdownRegex = /{{\s*?[#/][a-z-]+\s*?}}/g // - {% unless "bar" %} const stringInLiquidRegex = /{% (?:if|ifversion|elseif|unless) (?:"|').+?%}/g +// {% if version ... %} should be {% ifversion ... %} +const spaceInVersionTagRegex = /{% if version .+?%}/g + const relativeArticleLinkErrorText = 'Found unexpected relative article links:' const languageLinkErrorText = 'Found article links with hard-coded language codes:' const versionLinkErrorText = 'Found article links with hard-coded version numbers:' @@ -162,6 +165,7 @@ const oldVariableErrorText = 'Found article uses old {{ site.data... }} syntax. const oldOcticonErrorText = 'Found octicon variables with the old {{ octicon-name }} syntax. Use {% octicon "name" %} instead!' const oldExtendedMarkdownErrorText = 'Found extended markdown tags with the old {{#note}} syntax. Use {% note %}/{% endnote %} instead!' const stringInLiquidErrorText = 'Found Liquid conditionals that evaluate a string instead of a variable. Remove the quotes around the variable!' +const spaceInVersionTagErrorText = 'Found "if version" in Liquid markup. Use "ifversion" instead!' const mdWalkOptions = { globs: ['**/*.md'], @@ -419,6 +423,12 @@ describe('lint markdown content', () => { expect(matches.length, errorMessage).toBe(0) }) + test.only('does not contain Liquid statement with "if version"', async () => { + const matches = (content.match(spaceInVersionTagRegex) || []) + const errorMessage = formatLinkError(spaceInVersionTagErrorText, matches) + expect(matches.length, errorMessage).toBe(0) + }) + test('URLs must not contain a hard-coded language code', async () => { const matches = links.filter(link => { return /\/(?:${languageCodes.join('|')})\//.test(link) @@ -694,6 +704,22 @@ describe('lint yaml content', () => { const errorMessage = formatLinkError(stringInLiquidErrorText, matches) expect(matches.length, errorMessage).toBe(0) }) + + test.only('does not contain Liquid statement with "if version"', async () => { + const matches = [] + + for (const [key, content] of Object.entries(dictionary)) { + const contentStr = getContent(content) + if (!contentStr) continue + const valMatches = (contentStr.match(spaceInVersionTagRegex) || []) + if (valMatches.length > 0) { + matches.push(...valMatches.map((match) => `Key "${key}": ${match}`)) + } + } + + const errorMessage = formatLinkError(spaceInVersionTagErrorText, matches) + expect(matches.length, errorMessage).toBe(0) + }) } } ) From 37ed508ebeaa6096a0ad205bffd29a9558218e0b Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Thu, 10 Jun 2021 14:14:45 -0400 Subject: [PATCH 11/17] support not keyword and operator --- lib/liquid-tags/ifversion.js | 69 ++++++++++++++++++++++++++++++------ tests/unit/liquid.js | 58 +++++++++++++++++++++++++----- 2 files changed, 108 insertions(+), 19 deletions(-) diff --git a/lib/liquid-tags/ifversion.js b/lib/liquid-tags/ifversion.js index db39663943..430af5a380 100644 --- a/lib/liquid-tags/ifversion.js +++ b/lib/liquid-tags/ifversion.js @@ -6,11 +6,13 @@ const SyntaxHelp = "Syntax Error in 'ifversion' with range - Valid syntax: ifver const supportedOperators = [ '=', '<', - '>' + '>', + '!=' ] const supportedOperatorsRegex = new RegExp(`[${supportedOperators.join('')}]`) const releaseRegex = /\d\d?\.\d\d?/ +const notRegex = /(?:^|\s)not\s/ // This module supports a new tag we can use for docs versioning specifically. It extends the // native Liquid `if` block tag. It has special handling for statements like {% ifversion ghes < 3.0 %}, @@ -48,10 +50,18 @@ module.exports = { const r = this.liquid.renderer const { operators, operatorsTrie } = this.liquid.options + this.currentRelease = ctx.environments.currentVersion.split('@')[1] + this.currentVersionShortName = ctx.environments.allVersions[ctx.environments.currentVersion].shortName + for (const branch of this.branches) { + let resolvedBranchCond = branch.cond + + // Resolve "not" keywords in the conditional, if any. + resolvedBranchCond = this.handleNots(resolvedBranchCond) + // Resolve special operators in the conditional, if any. // This will replace syntax like `fpt or ghes < 3.0` with `fpt or true` or `fpt or false`. - const resolvedBranchCond = this.handleOperators(branch.cond, ctx.environments) + resolvedBranchCond = this.handleOperators(resolvedBranchCond) // Use Liquid's native function for the final evaluation. const cond = yield new Expression(resolvedBranchCond, operators, operatorsTrie, ctx.opts.lenientIf).value(ctx) @@ -64,7 +74,37 @@ module.exports = { yield r.renderTemplates(this.elseTemplates, ctx, emitter) }, - handleOperators (resolvedBranchCond, context) { + handleNots (resolvedBranchCond) { + if (!notRegex.test(resolvedBranchCond)) return resolvedBranchCond + + const condArray = resolvedBranchCond.split(' ') + + // Find the first index in the array that contains "not". + const notIndex = condArray.findIndex(el => el === 'not') + + // E.g., ['not', 'fpt'] + const condParts = condArray.slice(notIndex, notIndex + 2) + + // E.g., 'fpt' + const versionToEvaluate = condParts[1] + + // If the current version is the version being evaluated in the conditional, + // that is negated and resolved to false. If it's NOT the version being + // evaluated, that resolves to true. + const resolvedBoolean = !(versionToEvaluate === this.currentVersionShortName) + + // Replace syntax like `not fpt` with `true` or `false`. + resolvedBranchCond = resolvedBranchCond.replace(condParts.join(' '), resolvedBoolean) + + // Run this function recursively until we've resolved all the nots. + if (notRegex.test(resolvedBranchCond)) { + return this.handleNots(resolvedBranchCond) + } + + return resolvedBranchCond + }, + + handleOperators (resolvedBranchCond) { if (!supportedOperatorsRegex.test(resolvedBranchCond)) return resolvedBranchCond // If this conditional contains multiple parts using `or` or `and`, get only the conditional with operators. @@ -86,20 +126,27 @@ module.exports = { throw new TokenizationError(SyntaxHelp, this.tagToken) } - const currentRelease = context.currentVersion.split('@')[1] - const currentVersionShortName = context.allVersions[context.currentVersion].shortName - - // Evaluate the operator if this is the current version; otherwise it's always false. - const resolvedBoolean = versionShortName === currentVersionShortName - ? versionSatisfiesRange(currentRelease, `${operator}${releaseToEvaluate}`) - : false + let resolvedBoolean + if (operator === '!=') { + // If this is the current version, compare the release numbers. + // If it's not the current version, it's always true. + resolvedBoolean = versionShortName === this.currentVersionShortName + ? releaseToEvaluate !== this.currentRelease + : true + } else { + // If this is the current version, evaluate the operator using semver. + // If it's not the current version, it's always false. + resolvedBoolean = versionShortName === this.currentVersionShortName + ? versionSatisfiesRange(this.currentRelease, `${operator}${releaseToEvaluate}`) + : false + } // Replace syntax like `fpt or ghes < 3.0` with `fpt or true` or `fpt or false`. resolvedBranchCond = resolvedBranchCond.replace(condParts.join(' '), resolvedBoolean) // Run this function recursively until we've resolved all the special operators. if (supportedOperatorsRegex.test(resolvedBranchCond)) { - return this.handleOperators(resolvedBranchCond, context) + return this.handleOperators(resolvedBranchCond) } return resolvedBranchCond diff --git a/tests/unit/liquid.js b/tests/unit/liquid.js index 33323d278a..2ef7148489 100644 --- a/tests/unit/liquid.js +++ b/tests/unit/liquid.js @@ -9,14 +9,20 @@ const template = ` ` const shortVersionsTemplate = ` - {% ifversion fpt %} I am FPT {% endif %} - {% ifversion ghae %} I am GHAE {% endif %} - {% ifversion ghes %} I am GHES {% endif %} - {% ifversion ghes = 3.1 %} I am GHES = 3.1 {% endif %} - {% ifversion ghes > 3.1 %} I am GHES > 3.1 {% endif %} - {% ifversion ghes < 3.1 %} I am GHES < 3.1 {% endif %} - {% ifversion fpt or ghes < 3.0 %} I am FTP or GHES < 3.0 {% endif %} - {% ifversion ghes < 3.1 and ghes > 2.22 %} I am 3.0 only {% endif %} + {% ifversion fpt %} I am FPT {% endif %} + {% ifversion ghae %} I am GHAE {% endif %} + {% ifversion ghes %} I am GHES {% endif %} + {% ifversion ghes = 3.1 %} I am GHES = 3.1 {% endif %} + {% ifversion ghes > 3.1 %} I am GHES > 3.1 {% endif %} + {% ifversion ghes < 3.1 %} I am GHES < 3.1 {% endif %} + {% ifversion fpt or ghes < 3.0 %} I am FTP or GHES < 3.0 {% endif %} + {% ifversion ghes < 3.1 and ghes > 2.22 %} I am 3.0 only {% endif %} +` + +const negativeVersionsTemplate = ` + {% ifversion not ghae %} I am not GHAE {% endif %} + {% ifversion not ghes %} I am not GHES {% endif %} + {% ifversion ghes != 3.1 %} I am not GHES 3.1 {% endif %} ` describe('liquid template parser', () => { @@ -103,5 +109,41 @@ describe('liquid template parser', () => { const output = await liquid.parseAndRender(shortVersionsTemplate, req.context) expect(output.replace(/\s\s+/g, ' ').trim()).toBe('I am GHES I am GHES < 3.1 I am 3.0 only') }) + + test('NOT statements work as expected on versions without numbered releases', async () => { + req.context = { + currentVersion: 'github-ae@latest', + page: {}, + allVersions, + enterpriseServerReleases + } + await middleware(req, null, () => {}) + const output = await liquid.parseAndRender(negativeVersionsTemplate, req.context) + expect(output.replace(/\s\s+/g, ' ').trim()).toBe('I am not GHES I am not GHES 3.1') + }) + + test('NOT statements work as expected on versions with numbered releases', async () => { + req.context = { + currentVersion: 'enterprise-server@3.0', + page: {}, + allVersions, + enterpriseServerReleases + } + await middleware(req, null, () => {}) + const output = await liquid.parseAndRender(negativeVersionsTemplate, req.context) + expect(output.replace(/\s\s+/g, ' ').trim()).toBe('I am not GHAE I am not GHES 3.1') + }) + + test('The != operator works as expected', async () => { + req.context = { + currentVersion: 'enterprise-server@3.1', + page: {}, + allVersions, + enterpriseServerReleases + } + await middleware(req, null, () => {}) + const output = await liquid.parseAndRender(negativeVersionsTemplate, req.context) + expect(output.replace(/\s\s+/g, ' ').trim()).toBe('I am not GHAE') + }) }) }) From 5c2046987fb5d6c7f58b9dcee36ecf1014c7df20 Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Thu, 10 Jun 2021 14:16:00 -0400 Subject: [PATCH 12/17] comment --- lib/liquid-tags/ifversion.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/liquid-tags/ifversion.js b/lib/liquid-tags/ifversion.js index 430af5a380..08d657a59f 100644 --- a/lib/liquid-tags/ifversion.js +++ b/lib/liquid-tags/ifversion.js @@ -128,7 +128,7 @@ module.exports = { let resolvedBoolean if (operator === '!=') { - // If this is the current version, compare the release numbers. + // If this is the current version, compare the release numbers. (Our semver package doesn't handle !=.) // If it's not the current version, it's always true. resolvedBoolean = versionShortName === this.currentVersionShortName ? releaseToEvaluate !== this.currentRelease From b4b4804a0725df3edaf99e2e30b88ddd564c5d14 Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Fri, 11 Jun 2021 15:23:09 -0400 Subject: [PATCH 13/17] add ifversion tag linter --- tests/helpers/get-ifversion-conditionals.js | 17 +++ tests/linting/lint-files.js | 119 +++++++++++++++----- 2 files changed, 105 insertions(+), 31 deletions(-) create mode 100644 tests/helpers/get-ifversion-conditionals.js diff --git a/tests/helpers/get-ifversion-conditionals.js b/tests/helpers/get-ifversion-conditionals.js new file mode 100644 index 0000000000..8d6956a5a9 --- /dev/null +++ b/tests/helpers/get-ifversion-conditionals.js @@ -0,0 +1,17 @@ +const renderContent = require('../../lib/render-content') + +module.exports = function getIfversionConditionals (str) { + const conditionals = [] + + renderContent.liquid.parse(str) + .filter(block => block.name === 'ifversion') + // block.impl.branches is the only way to get an array of ifs and elsifs. + .map(block => block.impl.branches.map(branch => branch.cond)) + .forEach(block => { + block.forEach(branch => { + conditionals.push(branch) + }) + }) + + return conditionals +} diff --git a/tests/linting/lint-files.js b/tests/linting/lint-files.js index b367995fdd..b2d363a66e 100644 --- a/tests/linting/lint-files.js +++ b/tests/linting/lint-files.js @@ -15,8 +15,11 @@ const ghaeReleaseNotesSchema = require('../helpers/schemas/ghae-release-notes-sc const learningTracksSchema = require('../helpers/schemas/learning-tracks-schema') const renderContent = require('../../lib/render-content') const { execSync } = require('child_process') -const allVersions = Object.keys(require('../../lib/all-versions')) -const enterpriseServerVersions = allVersions.filter(v => v.startsWith('enterprise-server@')) +const allVersions = require('../../lib/all-versions') +const { supported } = require('../../lib/enterprise-server-releases') +const getIfversionConditionals = require('../helpers/get-ifversion-conditionals') +const enterpriseServerVersions = Object.keys(allVersions).filter(v => v.startsWith('enterprise-server@')) +const versionShortNames = Object.values(allVersions).map(v => v.shortName) const rootDir = path.join(__dirname, '../..') const contentDir = path.join(rootDir, 'content') @@ -29,6 +32,9 @@ const learningTracks = path.join(rootDir, 'data/learning-tracks') const languageCodes = Object.keys(languages) +const allowedVersionOperators = ['>','<','=','!='] +const versionShortNameExceptions = ['ghae-next', 'ghae-issue-'] + // WARNING: Complicated RegExp below! // // Things matched by this RegExp: @@ -151,9 +157,6 @@ const oldExtendedMarkdownRegex = /{{\s*?[#/][a-z-]+\s*?}}/g // - {% unless "bar" %} const stringInLiquidRegex = /{% (?:if|ifversion|elseif|unless) (?:"|').+?%}/g -// {% if version ... %} should be {% ifversion ... %} -const spaceInVersionTagRegex = /{% if version .+?%}/g - const relativeArticleLinkErrorText = 'Found unexpected relative article links:' const languageLinkErrorText = 'Found article links with hard-coded language codes:' const versionLinkErrorText = 'Found article links with hard-coded version numbers:' @@ -165,7 +168,6 @@ const oldVariableErrorText = 'Found article uses old {{ site.data... }} syntax. const oldOcticonErrorText = 'Found octicon variables with the old {{ octicon-name }} syntax. Use {% octicon "name" %} instead!' const oldExtendedMarkdownErrorText = 'Found extended markdown tags with the old {{#note}} syntax. Use {% note %}/{% endnote %} instead!' const stringInLiquidErrorText = 'Found Liquid conditionals that evaluate a string instead of a variable. Remove the quotes around the variable!' -const spaceInVersionTagErrorText = 'Found "if version" in Liquid markup. Use "ifversion" instead!' const mdWalkOptions = { globs: ['**/*.md'], @@ -282,7 +284,8 @@ describe('lint markdown content', () => { describe.each(mdToLint)( '%s', (markdownRelPath, markdownAbsPath) => { - let content, ast, links, yamlScheduledWorkflows, isHidden, isEarlyAccess, isSitePolicy, frontmatterErrors, frontmatterData + let content, ast, links, yamlScheduledWorkflows, isHidden, isEarlyAccess, isSitePolicy, frontmatterErrors, frontmatterData, + ifversionConditionals beforeAll(async () => { const fileContents = await readFileAsync(markdownAbsPath, 'utf8') @@ -317,6 +320,9 @@ describe('lint markdown content', () => { }))) .flat() .map(schedule => schedule.cron) + + ifversionConditionals = getIfversionConditionals(data) + .concat(getIfversionConditionals(bodyContent)) }) // We need to support some non-Early Access hidden docs in Site Policy @@ -326,6 +332,11 @@ describe('lint markdown content', () => { } }) + test.only('ifversion conditionals are valid in markdown', async () => { + const errors = validateIfversionConditionals(ifversionConditionals) + expect(errors.length, errors.join('\n')).toBe(0) + }) + test('relative URLs must start with "/"', async () => { const matches = links.filter(link => { if ( @@ -423,12 +434,6 @@ describe('lint markdown content', () => { expect(matches.length, errorMessage).toBe(0) }) - test.only('does not contain Liquid statement with "if version"', async () => { - const matches = (content.match(spaceInVersionTagRegex) || []) - const errorMessage = formatLinkError(spaceInVersionTagErrorText, matches) - expect(matches.length, errorMessage).toBe(0) - }) - test('URLs must not contain a hard-coded language code', async () => { const matches = links.filter(link => { return /\/(?:${languageCodes.join('|')})\//.test(link) @@ -507,15 +512,22 @@ describe('lint yaml content', () => { describe.each(ymlToLint)( '%s', (yamlRelPath, yamlAbsPath) => { - let dictionary, isEarlyAccess + let dictionary, isEarlyAccess, ifversionConditionals beforeAll(async () => { const fileContents = await readFileAsync(yamlAbsPath, 'utf8') + ifversionConditionals = getIfversionConditionals(fileContents) + dictionary = yaml.load(fileContents, { filename: yamlRelPath }) isEarlyAccess = yamlRelPath.split('/').includes('early-access') }) + test.only('ifversion conditionals are valid in yaml', async () => { + const errors = validateIfversionConditionals(ifversionConditionals) + expect(errors.length, errors.join('\n')).toBe(0) + }) + test('relative URLs must start with "/"', async () => { const matches = [] @@ -704,22 +716,6 @@ describe('lint yaml content', () => { const errorMessage = formatLinkError(stringInLiquidErrorText, matches) expect(matches.length, errorMessage).toBe(0) }) - - test.only('does not contain Liquid statement with "if version"', async () => { - const matches = [] - - for (const [key, content] of Object.entries(dictionary)) { - const contentStr = getContent(content) - if (!contentStr) continue - const valMatches = (contentStr.match(spaceInVersionTagRegex) || []) - if (valMatches.length > 0) { - matches.push(...valMatches.map((match) => `Key "${key}": ${match}`)) - } - } - - const errorMessage = formatLinkError(spaceInVersionTagErrorText, matches) - expect(matches.length, errorMessage).toBe(0) - }) } } ) @@ -846,7 +842,7 @@ describe('lint learning tracks', () => { const featuredTracks = {} const context = { enterpriseServerVersions } - await Promise.all(allVersions.map(async (version) => { + await Promise.all(Object.keys(allVersions).map(async (version) => { const featuredTracksPerVersion = (await Promise.all(Object.values(dictionary).map(async (entry) => { if (!entry.featured_track) return context.currentVersion = version @@ -880,3 +876,64 @@ describe('lint learning tracks', () => { } ) }) + +function validateVersion (version) { + return versionShortNames.includes(version) || + versionShortNameExceptions.some(exception => version.startsWith(exception)) +} + +function validateIfversionConditionals (conds) { + const errors = [] + + conds.forEach(cond => { + // This will get us an array of strings, where each string may have these space-separated parts: + // * Length 1: `` (example: `fpt`) + // * Length 2: `not ` (example: `not ghae`) + // * Length 3: ` ` (example: `ghes > 3.0`) + const condParts = cond + .split(/ (or|and) /) + .filter(part => !(part === 'or' || part === 'and')) + + condParts + .forEach(str => { + const strParts = str.split(' ') + // if length = 1, this should be a valid short version name. + if (strParts.length === 1) { + const version = strParts[0] + const isValidVersion = validateVersion(version) + if (!isValidVersion) { + errors.push(`"${version}" is not a valid short version name`) + } + } + + // if length = 2, this should be 'not' followed by a valid short version name. + if (strParts.length === 2) { + const [notKeyword, version] = strParts + const isValidVersion = validateVersion(version) + const isValid = notKeyword === 'not' && isValidVersion + if (!isValid) { + errors.push(`"${cond}" is not a valid conditional`) + } + } + + // if length = 3, this should be a range in the format: ghes > 3.0 + // where the first item is `ghes` (currently the only version with numbered releases), + // the second item is a supported operator, and the third is a supported GHES release. + if (strParts.length === 3) { + const [version, operator, release] = strParts + const isGhes = version === 'ghes' + const isSupportedOperator = allowedVersionOperators.includes(operator) + const isSupportedRelease = supported.includes(release) + const isValid = isGhes && isSupportedOperator && isSupportedRelease + const errorMessage = str === cond + ? `"${str}" is not a valid operation` + : `"${str}" is not a valid operation inside "${cond}"` + if (!isValid) { + errors.push(errorMessage) + } + } + }) + }) + + return errors +} From 5d7174d0350a7c6c47c6d0cf42b490f1b19a2781 Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Fri, 11 Jun 2021 15:32:01 -0400 Subject: [PATCH 14/17] remove .only --- tests/linting/lint-files.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/linting/lint-files.js b/tests/linting/lint-files.js index 42d184ac64..a4a3dc00bc 100644 --- a/tests/linting/lint-files.js +++ b/tests/linting/lint-files.js @@ -333,7 +333,7 @@ describe('lint markdown content', () => { } }) - test.only('ifversion conditionals are valid in markdown', async () => { + test('ifversion conditionals are valid in markdown', async () => { const errors = validateIfversionConditionals(ifversionConditionals) expect(errors.length, errors.join('\n')).toBe(0) }) @@ -524,7 +524,7 @@ describe('lint yaml content', () => { isEarlyAccess = yamlRelPath.split('/').includes('early-access') }) - test.only('ifversion conditionals are valid in yaml', async () => { + test('ifversion conditionals are valid in yaml', async () => { const errors = validateIfversionConditionals(ifversionConditionals) expect(errors.length, errors.join('\n')).toBe(0) }) From f3312199d8746ce8025a4ba13716d6a08dd1d1b2 Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Mon, 14 Jun 2021 15:33:18 -0400 Subject: [PATCH 15/17] Update middleware/contextualizers/short-versions.js --- middleware/contextualizers/short-versions.js | 1 - 1 file changed, 1 deletion(-) diff --git a/middleware/contextualizers/short-versions.js b/middleware/contextualizers/short-versions.js index 0957558a08..d9715fbf3a 100644 --- a/middleware/contextualizers/short-versions.js +++ b/middleware/contextualizers/short-versions.js @@ -7,7 +7,6 @@ // // For the custom operator handling in statements like {% if ghes > 3.0 %}, see `lib/liquid-tags/if-ver.js`. module.exports = async function shortVersions (req, res, next) { - if (!req.context.page) return next() const { allVersions, currentVersion } = req.context const currentVersionObj = allVersions[currentVersion] From d96fa5e2f03cbf751e4af78cf5d513ddf226f54f Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Mon, 14 Jun 2021 16:24:48 -0400 Subject: [PATCH 16/17] add convience props to context and put supported ops in a separate module --- lib/liquid-tags/ifversion-supported-operators.js | 6 ++++++ lib/liquid-tags/ifversion.js | 12 +++--------- middleware/contextualizers/short-versions.js | 4 ++++ tests/linting/lint-files.js | 2 +- 4 files changed, 14 insertions(+), 10 deletions(-) create mode 100644 lib/liquid-tags/ifversion-supported-operators.js diff --git a/lib/liquid-tags/ifversion-supported-operators.js b/lib/liquid-tags/ifversion-supported-operators.js new file mode 100644 index 0000000000..13b6c6050f --- /dev/null +++ b/lib/liquid-tags/ifversion-supported-operators.js @@ -0,0 +1,6 @@ +module.exports = [ + '=', + '<', + '>', + '!=' +] diff --git a/lib/liquid-tags/ifversion.js b/lib/liquid-tags/ifversion.js index 08d657a59f..bc35a8f29e 100644 --- a/lib/liquid-tags/ifversion.js +++ b/lib/liquid-tags/ifversion.js @@ -1,15 +1,9 @@ const { isTruthy, Expression, TokenizationError } = require('liquidjs') const versionSatisfiesRange = require('../version-satisfies-range') +const supportedOperators = require('./ifversion-supported-operators') const SyntaxHelp = "Syntax Error in 'ifversion' with range - Valid syntax: ifversion [operator] [releaseNumber]" -const supportedOperators = [ - '=', - '<', - '>', - '!=' -] - const supportedOperatorsRegex = new RegExp(`[${supportedOperators.join('')}]`) const releaseRegex = /\d\d?\.\d\d?/ const notRegex = /(?:^|\s)not\s/ @@ -50,8 +44,8 @@ module.exports = { const r = this.liquid.renderer const { operators, operatorsTrie } = this.liquid.options - this.currentRelease = ctx.environments.currentVersion.split('@')[1] - this.currentVersionShortName = ctx.environments.allVersions[ctx.environments.currentVersion].shortName + this.currentRelease = ctx.environments.currentRelease + this.currentVersionShortName = ctx.environments.currentVersionShortName for (const branch of this.branches) { let resolvedBranchCond = branch.cond diff --git a/middleware/contextualizers/short-versions.js b/middleware/contextualizers/short-versions.js index d9715fbf3a..dfab511b54 100644 --- a/middleware/contextualizers/short-versions.js +++ b/middleware/contextualizers/short-versions.js @@ -15,5 +15,9 @@ module.exports = async function shortVersions (req, res, next) { // Add the short name to context. req.context[currentVersionObj.shortName] = true + // Add convenience props. + req.context.currentRelease = currentVersion.split('@')[1] + req.context.currentVersionShortName = currentVersionObj.shortName + return next() } diff --git a/tests/linting/lint-files.js b/tests/linting/lint-files.js index a4a3dc00bc..0b0497fe6f 100644 --- a/tests/linting/lint-files.js +++ b/tests/linting/lint-files.js @@ -21,6 +21,7 @@ const { supported } = require('../../lib/enterprise-server-releases') const getIfversionConditionals = require('../helpers/get-ifversion-conditionals') const enterpriseServerVersions = Object.keys(allVersions).filter(v => v.startsWith('enterprise-server@')) const versionShortNames = Object.values(allVersions).map(v => v.shortName) +const allowedVersionOperators = require('../../lib/liquid-tags/ifversion-supported-operators') const rootDir = path.join(__dirname, '../..') const contentDir = path.join(rootDir, 'content') @@ -33,7 +34,6 @@ const learningTracks = path.join(rootDir, 'data/learning-tracks') const languageCodes = Object.keys(languages) -const allowedVersionOperators = ['>','<','=','!='] const versionShortNameExceptions = ['ghae-next', 'ghae-issue-'] // WARNING: Complicated RegExp below! From 8ed8a8e68bd986fa65fb54068222545cae3cd1ba Mon Sep 17 00:00:00 2001 From: Sarah Schneider Date: Mon, 14 Jun 2021 16:27:22 -0400 Subject: [PATCH 17/17] add comments to clarify what is borrowed and what is an override --- lib/liquid-tags/ifversion.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/liquid-tags/ifversion.js b/lib/liquid-tags/ifversion.js index bc35a8f29e..2101de78c7 100644 --- a/lib/liquid-tags/ifversion.js +++ b/lib/liquid-tags/ifversion.js @@ -13,6 +13,7 @@ const notRegex = /(?:^|\s)not\s/ // using semver to evaluate release numbers instead of doing standard number comparisons, which // don't work the way we want because they evaluate 3.2 > 3.10 = true. module.exports = { + // The following is verbatim from https://github.com/harttle/liquidjs/blob/v9.22.1/src/builtin/tags/if.ts parse (tagToken, remainTokens) { this.tagToken = tagToken this.branches = [] @@ -40,6 +41,8 @@ module.exports = { stream.start() }, + // The following is _mostly_ verbatim from https://github.com/harttle/liquidjs/blob/v9.22.1/src/builtin/tags/if.ts + // The additions here are the handleNots() and handleOperators() calls. render: function * (ctx, emitter) { const r = this.liquid.renderer const { operators, operatorsTrie } = this.liquid.options