improved warning message for relay.id

happylinks · Nov 3, 2017 · 0a9f7cd · 0a9f7cd
1 parent 4ac22ce
commit 0a9f7cd
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 29 deletions.
diff --git a/README.md b/README.md
@@ -94,7 +94,7 @@ Checks if Types ending with Connection have 'edges' and 'pageInfo'.
 ### Relay ID
 Checks if fields that have 'id' in them use the ID type instead of others (string, integer, etc.)
 
-`Field 'id' uses String. Please use 'ID' instead.`
+`Field 'id' in Type 'User' uses 'String'. Please use 'ID' instead.`
 
 ### Singular Mutations
 Checks if mutations are singular (createUser) and not plural (createUsers).

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "gqlint",
-  "version": "1.6.0",
+  "version": "1.6.1",
   "description": "GraphQL Linter",
   "keywords": [
     "graphql",

diff --git a/rules/relay.id.js b/rules/relay.id.js
@@ -1,20 +1,22 @@
 const { visit, getLocation } = require('graphql/language');
 
+const getMessage = (text, node, fieldName, typeName) => {
+  const message = `Field '${fieldName}' in Type '${typeName}' uses '${node.name
+    .value}'. Please use 'ID' instead.`;
+  const location = getLocation({ body: text }, node.name.loc.start);
+
+  return {
+    message,
+    line: location.line,
+    column: location.column,
+    ruleId: 'relay.id',
+    severity: 1
+  };
+};
+
 // Check if type has a valid id type.
-const checkType = (text, fieldName, node) => {
-  if (node.name.value !== 'ID' && fieldName !== 'clientMutationId') {
-    const message = `Field '${fieldName}' uses ${node.name
-      .value}. Please use 'ID' instead.`;
-    const location = getLocation({ body: text }, node.name.loc.start);
-
-    return {
-      message,
-      line: location.line,
-      column: location.column,
-      ruleId: 'relay.id',
-      severity: 1
-    };
-  }
+const isValidType = (text, fieldName, node) => {
+  return !(node.name.value !== 'ID' && fieldName !== 'clientMutationId');
 };
 
 // Split camelcase and turn into array.
@@ -25,30 +27,43 @@ const splitFieldName = fieldName =>
     .map(value => value.toLowerCase());
 
 // Walk through this field until you find the NamedType, and check if that type is valid.
-const checkNamedType = (node, text, messages) => {
+const checkNamedType = (node, text, typeName, messages) => {
   const fieldName = node.name.value;
   const fieldNameSplit = splitFieldName(fieldName);
   if (fieldNameSplit.includes('id')) {
     visit(node, {
       NamedType(node) {
-        const message = checkType(text, fieldName, node);
-        if (message) {
+        const valid = isValidType(text, fieldName, node);
+        if (!valid) {
+          const message = getMessage(text, node, fieldName, typeName);
           messages.push(message);
         }
       }
     });
   }
 };
 
+const checkTypeDefinition = (node, text, messages) => {
+  const typeName = node.name.value;
+  visit(node, {
+    FieldDefinition(node) {
+      checkNamedType(node, text, typeName, messages);
+    },
+    InputValueDefinition(node) {
+      checkNamedType(node, text, typeName, messages);
+    }
+  });
+};
+
 module.exports = function(ast, text) {
   const messages = [];
 
   visit(ast, {
-    InputValueDefinition(node) {
-      checkNamedType(node, text, messages);
+    ObjectTypeDefinition(node) {
+      checkTypeDefinition(node, text, messages);
     },
-    FieldDefinition(node) {
-      checkNamedType(node, text, messages);
+    InputObjectTypeDefinition(node) {
+      checkTypeDefinition(node, text, messages);
     }
   });
 

diff --git a/tests/rules/relay.id.test.js b/tests/rules/relay.id.test.js
@@ -38,42 +38,42 @@ describe('Rule: Relay ID', () => {
     const results = gqLint(invalid.uuid, '', {});
     expect(results[0].warningCount).toBe(1);
     expect(results[0].messages[0].message).toBe(
-      `Field 'id' uses UUID. Please use 'ID' instead.`
+      `Field 'id' in Type 'User' uses 'UUID'. Please use 'ID' instead.`
     );
   });
   test('GlobalID', () => {
     const results = gqLint(invalid.globalId, '', {});
     expect(results[0].warningCount).toBe(1);
     expect(results[0].messages[0].message).toBe(
-      `Field 'id' uses GlobalID. Please use 'ID' instead.`
+      `Field 'id' in Type 'User' uses 'GlobalID'. Please use 'ID' instead.`
     );
   });
   test('String', () => {
     const results = gqLint(invalid.string, '', {});
     expect(results[0].warningCount).toBe(1);
     expect(results[0].messages[0].message).toBe(
-      `Field 'id' uses String. Please use 'ID' instead.`
+      `Field 'id' in Type 'User' uses 'String'. Please use 'ID' instead.`
     );
   });
   test('StrangeId', () => {
     const results = gqLint(invalid.strangeId, '', {});
     expect(results[0].warningCount).toBe(1);
     expect(results[0].messages[0].message).toBe(
-      `Field 'id' uses StrangeIdType. Please use 'ID' instead.`
+      `Field 'id' in Type 'User' uses 'StrangeIdType'. Please use 'ID' instead.`
     );
   });
   test('Custom fieldName', () => {
     const results = gqLint(invalid.customFieldName, '', {});
     expect(results[0].warningCount).toBe(1);
     expect(results[0].messages[0].message).toBe(
-      `Field 'relatedId' uses String. Please use 'ID' instead.`
+      `Field 'relatedId' in Type 'User' uses 'String'. Please use 'ID' instead.`
     );
   });
   test('Input', () => {
     const results = gqLint(invalid.input, '', {});
     expect(results[0].warningCount).toBe(1);
     expect(results[0].messages[0].message).toBe(
-      `Field 'id' uses GlobalResource. Please use 'ID' instead.`
+      `Field 'id' in Type 'UserInput' uses 'GlobalResource'. Please use 'ID' instead.`
     );
   });
 });