Update globals check (#489)

no refs
- log error for each bad reference instead of one per file
- move to top-level global checking (less specific for the moment)
- add all data variables in lieu of checking within foreach loop

lib/ast-linter/rules/internal/scope.js

@@ -1,68 +1,33 @@
 const {getNodeName} = require('../../helpers');
 const _ = require('lodash');
 
-const globals = {
-    site: { // https://ghost.org/docs/themes/helpers/site/
-        accent_color: true,
-        comments_enabled: true, // no docs???
-        cover_image: true,
-        description: true,
-        facebook: true,
-        icon: true,
-        locale: true,
-        logo: true,
-        navigation: true,
-        secondary_navigation: true, // no docs???
-        signup_url: true,
-        timezone: true,
-        title: true,
-        twitter: true,
-        url: true,
-        // feature flags
-        members_enabled: true,
-        members_invite_only: true,
-        paid_members_enabled: true,
-        // meta data
-        meta_title: true,
-        meta_description: true,
-        twitter_image: true,
-        twitter_title: true,
-        twitter_description: true,
-        og_image: true,
-        og_title: true,
-        og_description: true
-    },
-    member: { // https://ghost.org/docs/themes/members/#member-attributes
-        email: true,
-        name: true,
-        firstname: true,
-        uuid: true,
-        paid: true,
-        subscriptions: true
-    },
-    setting: {
-        paid_members_enabled: true // TODO: remove; pretty certain this should just be site.paid_members_enabled, issue with Journal theme
-    },
-    config: {
-        posts_per_page: true
-    },
-    labs: {
-        publicAPI: true,
-        subscribers: true
-    },
-    // handlebars vars
+// TODO: the allowlist should include all properties of these top-level globals
+const ghostGlobals = {
+    site: true,
+    member: true,
+    setting: true, // TODO: we should remove this but the Journal theme is using it atm
+    config: true,
+    labs: true,
+    custom: true
+};
+
+// unless we update AST to check that we're within a foreach block, we need to allowlist all of these as they look the same as globals
+const dataVars = {
+    index: true,
+    number: true,
+    key: true,
     first: true,
     last: true,
+    odd: true,
     even: true,
-    odd: true
+    rowStart: true,
+    rowEnd: true
 };
 
 // unless we move from lodash to a glob match, we just need to handle all custom since we can't allowlist those
-function isGlobal(parts) {
-    if (parts && parts[0] === 'custom') {
-        return true;
-    }
-    return _.get(globals, parts.join('.'));
+function isOnAllowlist(parts) {
+    const variable = parts && parts[0];
+    return ghostGlobals[variable] || dataVars[variable] || false;
 }
 
 // true = property exists
@@ -259,18 +224,18 @@ class Scope {
         return matchedFrame && matchedFrame.node;
     }
 
-    isLocal(node) {
+    isKnownVariable(node) {
         // @foo statements are referencing globals rather than locals
         //  and can be detected with the data: true attribute (???)
 
         // they can be direct [Mustache] statements {{@foo}}...
         if (node.type === 'MustacheStatement' && node.path.data) {
-            return isGlobal(node.path.parts);
+            return isOnAllowlist(node.path.parts);
         }
 
         // ... or indirect using helpers, e.g. {{#match @foo.bar}}{{/match}}
         if (node.type === 'PathExpression') {
-            return isGlobal(node.parts);
+            return isOnAllowlist(node.parts);
         }
 
         let name = getNodeName(node);

lib/ast-linter/rules/lint-no-unknown-globals.js

@@ -3,7 +3,7 @@ const {logNode} = require('../helpers');
 
 module.exports = class NoUnknownGlobals extends Rule {
     _checkMustacheForUnknownGlobal(node) {
-        if (node.path.data && !this.scope.isLocal(node)) {
+        if (node.path.data && !this.scope.isKnownVariable(node)) {
             this.log({
                 message: `${logNode(node)} is not a known global`,
                 line: node.loc && node.loc.start.line,
@@ -16,7 +16,7 @@ module.exports = class NoUnknownGlobals extends Rule {
     _checkBlockForUnknownGlobal(node) {
         if (node.path.type === 'PathExpression') {
             node.params.forEach((param) => {
-                if (param.data && !this.scope.isLocal(param)) {
+                if (param.data && !this.scope.isKnownVariable(param)) {
                     this.log({
                         message: `${logNode(param)} is not a known global`,
                         line: param.loc && param.loc.start.line,

lib/checks/120-no-unknown-globals.js

@@ -42,9 +42,11 @@ function processFileFunction(files, failures, theme, partialsFound) {
         });
 
         if (astResults.length) {
-            failures.push({
-                ref: themeFile.file,
-                message: astResults[0].message
+            astResults.forEach((result) => {
+                failures.push({
+                    ref: themeFile.file,
+                    message: result.message
+                });
             });
         }
 

test/120-no-unknown-globals.test.js

@@ -6,13 +6,14 @@ describe('120 No unknown globals', function () {
     describe('v5', function () {
         const options = {checkVersion: 'v5'};
 
-        it('should detect unknown globals', function (done) {
-            utils.testCheck(thisCheck, '120-no-unknown-globals/v5/invalid-with-globals', options).then(function (output) {
+        it.only('should detect unknown globals', function (done) {
+            utils.testCheck(thisCheck, '120-no-unknown-globals/v5/invalid', options).then(function (output) {
                 output.should.be.a.ValidThemeObject();
 
                 // should be a warning about unused globals
                 output.results.fail.should.be.an.Object().with.keys('GS120-NO-UNKNOWN-GLOBALS');
                 output.results.fail['GS120-NO-UNKNOWN-GLOBALS'].should.be.a.ValidFailObject();
+                output.results.fail['GS120-NO-UNKNOWN-GLOBALS'].failures.should.be.an.Array().with.lengthOf(4);
 
                 done();
             }).catch(done);
@@ -29,7 +30,7 @@ describe('120 No unknown globals', function () {
             }).catch(done);
         });
 
-        it('should pass specific locals {{@first}}', function (done) {
+        it('should pass specific data variables like {{@first}}', function (done) {
             utils.testCheck(thisCheck, '120-no-unknown-globals/v5/valid-with-locals', options).then(function (output) {
                 output.should.be.a.ValidThemeObject();
 

test/fixtures/themes/120-no-unknown-globals/v5/invalid-with-globals/default.hbs → test/fixtures/themes/120-no-unknown-globals/v5/invalid/default.hbs

@@ -6,7 +6,8 @@
     {{ghost_head}}
 </head>
 <body>
-
+    {{@prop}}
+    {{#match @test}}{{/match}}
     <h1>{{@blog.title}}</h1>
     {{ghost_foot}}
 </body>

test/fixtures/themes/120-no-unknown-globals/v5/valid-with-locals/default.hbs

@@ -9,7 +9,16 @@
                 <div class="subscriber-header">
                     <div class="subscription-title">
                         {{#if @first}}
-                            <h1>Your account</h1>
+                            <h1>First</h1>
+                        {{/if}}
+                        {{#if @last}}
+                            <h2>Last</h2>
+                        {{/if}}
+                        {{#if @even}}
+                            <h3>Even</h3>
+                        {{/if}}
+                        {{#if @odd}}
+                            <h4>Odd</h4>
                         {{/if}}
                     </div>
                 </div>