From 70f756738ce069b9fffef3d067af2b5f32737709 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Thu, 4 Nov 2021 20:31:36 -0400 Subject: [PATCH] Add optional static enum lint (#36759) * Add optional static enum lint Partial for #36754 * Demonstrate with changes * Exempt tests from enums linting * Apply suggestions from code review --- .eslintrc.js | 2 + build-system/.eslintrc.js | 1 + build-system/eslint-rules/enums.js | 187 ++++++++++++++++++ src/amp-story-player/amp-story-player-impl.js | 118 ++++++----- 4 files changed, 256 insertions(+), 52 deletions(-) create mode 100644 build-system/eslint-rules/enums.js diff --git a/.eslintrc.js b/.eslintrc.js index e3209e75c4b5..5457c9ae8f8a 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -166,6 +166,7 @@ module.exports = { 'local/camelcase': 2, 'local/closure-type-primitives': 2, 'local/dict-string-keys': 2, + 'local/enums': 2, 'local/get-mode-usage': 2, 'local/html-template': 2, 'local/is-experiment-on': 2, @@ -355,6 +356,7 @@ module.exports = { 'rules': { 'require-jsdoc': 0, 'local/always-call-chai-methods': 2, + 'local/enums': 0, 'local/no-bigint': 0, 'local/no-dynamic-import': 0, 'local/no-function-async': 0, diff --git a/build-system/.eslintrc.js b/build-system/.eslintrc.js index 760a84ec1132..d89f2946f7dd 100644 --- a/build-system/.eslintrc.js +++ b/build-system/.eslintrc.js @@ -22,6 +22,7 @@ module.exports = { }, 'rules': { 'import/no-unresolved': 0, + 'local/enums': 0, 'local/no-bigint': 0, 'local/no-dynamic-import': 0, 'local/no-export-side-effect': 0, diff --git a/build-system/eslint-rules/enums.js b/build-system/eslint-rules/enums.js new file mode 100644 index 000000000000..6490e500dcc9 --- /dev/null +++ b/build-system/eslint-rules/enums.js @@ -0,0 +1,187 @@ +'use strict'; + +/** + * Enforces all enum usage is staticly DCE-able. + * + * This is an opt-in lint, with enums enabling by using a "_Enum" suffix. + * + * Good + * ``` + * /** @enum *\/ + * const Foo_Enum = { + * ABC: 1, + * DEF: 2, + * }; + * + * const { ABC: value } = Foo_Enum; + * const value = Foo_Enum.ABC; + * isEnumValue(Foo_Enum, value); + * ``` + * + * Bad + * ``` + * const Bar_Enum = { + * 0: 0, + * '1': 1, + * ['2'] : 2, + * [ref]: 4, + * }; + * + * Bar_Enum[0]; + * Bar_Enum['0']; + * Bar_Enum['key']; + * Bar_Enum[ref]; + * key in Bar_Enum; + * ``` + * + * @return {!Object} + */ +module.exports = function (context) { + /** + * @param {!Node} node + * @return {boolean} + */ + function hasEnumAnnotation(node) { + const commentLines = context.getCommentsBefore(node); + if (!commentLines) { + return false; + } + + return commentLines.some(function (comment) { + return comment.type == 'Block' && /@enum/.test(comment.value); + }); + } + + /** + * @param {!Node|undefined} node + */ + function checkEnumId(node) { + if (/^[A-Z](?:[A-Za-z0-9]+_Enum|[A-Z0-9_]+_ENUM)$/.test(node.name)) { + return; + } + context.report({ + node, + message: + 'Enums should use PascalCase and end in "_Enum", or SCREAMING_SNAKE_CASE and end in "_ENUM"', + }); + } + + /** + * @param {!Node|undefined} node + */ + function checkEnumKeys(node) { + for (const prop of node.properties) { + if (prop.computed || prop.key?.type !== 'Identifier') { + context.report({ + node: prop, + message: [ + 'Enum keys must be a normal prop identifier', + 'eg, `{ KEY: value }` ', + ].join('\n\t'), + }); + } + } + } + + /** + * @param {!Node} node + */ + function checkStaticEnumUse(node) { + const {parent} = node; + // import { Enum } from '.' + if (parent.type === 'ImportSpecifier') { + return; + } + + if (parent.type === 'VariableDeclarator') { + // const Enum = {} + if (parent.id === node) { + // Check via the VariableDeclaration visitor. + return; + } + + // const { key } = Enum + if (parent.init === node && parent.id.type === 'ObjectPattern') { + checkEnumKeys(parent.id); + return; + } + } + + if (parent.type === 'MemberExpression') { + // Enum.key get + if (parent.object === node && parent.computed === false) { + return; + } + } + + if (parent.type === 'CallExpression') { + const {arguments: args, callee} = parent; + if (args[0] === node) { + // isEnumValue(Enum, value) + if (callee.type === 'Identifier') { + const {name} = callee; + if (name === 'isEnumValue') { + return; + } + } + } + } + + context.report({ + node, + message: [ + `Improper use of enum, you may only do:`, + `- \`${node.name}.key\` get access.`, + `- \`isEnumValue(${node.name}, someValue)\` value checks.`, + ].join('\n\t'), + }); + } + + return { + Identifier(node) { + if (/_E(NUM|num)$/.test(node.name)) { + checkStaticEnumUse(node); + } + }, + + VariableDeclaration(node) { + const {declarations} = node; + if (declarations.length !== 1) { + return; + } + const decl = declarations[0]; + const {id, init} = decl; + if (id.type !== 'Identifier') { + return; + } + if (!/_E(NUM|num)$/.test(id.name)) { + return; + } + + let annotationNode = node; + const {parent} = node; + if (parent.type === 'ExportNamedDeclaration') { + annotationNode = parent; + } + if (!hasEnumAnnotation(annotationNode)) { + context.report({ + node: annotationNode, + message: 'Static enums require an @enum annotation', + }); + } + + checkEnumId(id); + if (init?.type === 'ObjectExpression') { + checkEnumKeys(init); + } else { + context.report({ + node: decl.init || decl, + message: [ + 'Static enums must be initialized with at literal object expression', + `eg, \`${node.kind} ${id.name} = { … }\``, + ].join('\n\t'), + }); + } + }, + }; +}; diff --git a/src/amp-story-player/amp-story-player-impl.js b/src/amp-story-player/amp-story-player-impl.js index 01a76378ea96..7cdedc5c3120 100644 --- a/src/amp-story-player/amp-story-player-impl.js +++ b/src/amp-story-player/amp-story-player-impl.js @@ -8,6 +8,7 @@ import {Deferred} from '#core/data-structures/promise'; import {isJsonScriptTag, tryFocus} from '#core/dom'; import {resetStyles, setStyle, setStyles} from '#core/dom/style'; import {findIndex, toArray} from '#core/types/array'; +import {isEnumValue} from '#core/types/enum'; import {dict} from '#core/types/object'; import {parseJson} from '#core/types/object/json'; import {parseQueryString} from '#core/types/string/url'; @@ -33,14 +34,14 @@ import { } from '../url'; /** @enum {string} */ -const LoadStateClass = { +const LOAD_STATE_CLASS_ENUM = { LOADING: 'i-amphtml-story-player-loading', LOADED: 'i-amphtml-story-player-loaded', ERROR: 'i-amphtml-story-player-error', }; /** @enum {number} */ -const StoryPosition = { +const STORY_POSITION_ENUM = { PREVIOUS: -1, CURRENT: 0, NEXT: 1, @@ -53,7 +54,7 @@ const SUPPORTED_CACHES = ['cdn.ampproject.org', 'www.bing-amp.com']; const SANDBOX_MIN_LIST = ['allow-top-navigation']; /** @enum {number} */ -const SwipingState = { +const SWIPING_STATE_ENUM = { NOT_SWIPING: 0, SWIPING_TO_LEFT: 1, SWIPING_TO_RIGHT: 2, @@ -69,32 +70,32 @@ const TOGGLE_THRESHOLD_PX = 50; const FETCH_STORIES_THRESHOLD = 2; /** @enum {string} */ -const DEPRECATED_BUTTON_TYPES = { +const DEPRECATED_BUTTON_TYPES_ENUM = { BACK: 'back-button', CLOSE: 'close-button', }; /** @enum {string} */ -const DEPRECATED_BUTTON_CLASSES = { +const DEPRECATED_BUTTON_CLASSES_ENUM = { BASE: 'amp-story-player-exit-control-button', HIDDEN: 'amp-story-player-hide-button', - [DEPRECATED_BUTTON_TYPES.BACK]: 'amp-story-player-back-button', - [DEPRECATED_BUTTON_TYPES.CLOSE]: 'amp-story-player-close-button', + BACK: 'amp-story-player-back-button', + CLOSE: 'amp-story-player-close-button', }; /** @enum {string} */ -const DEPRECATED_EVENT_NAMES = { - [DEPRECATED_BUTTON_TYPES.BACK]: 'amp-story-player-back', - [DEPRECATED_BUTTON_TYPES.CLOSE]: 'amp-story-player-close', +const DEPRECATED_EVENT_NAMES_ENUM = { + BACK: 'amp-story-player-back', + CLOSE: 'amp-story-player-close', }; /** @enum {string} */ -const STORY_STATE_TYPE = { +const STORY_STATE_TYPE_ENUM = { PAGE_ATTACHMENT_STATE: 'page-attachment', }; /** @enum {string} */ -const STORY_MESSAGE_STATE_TYPE = { +const STORY_MESSAGE_STATE_TYPE_ENUM = { PAGE_ATTACHMENT_STATE: 'PAGE_ATTACHMENT_STATE', UI_STATE: 'UI_STATE', MUTED_STATE: 'MUTED_STATE', @@ -170,7 +171,7 @@ export let ViewerControlDef; const TAG = 'amp-story-player'; /** @enum {string} */ -const LOG_TYPE = { +const LOG_TYPE_ENUM = { DEV: 'amp-story-player-dev', }; @@ -211,8 +212,8 @@ export class AmpStoryPlayer { /** @private {number} */ this.currentIdx_ = 0; - /** @private {!SwipingState} */ - this.swipingState_ = SwipingState.NOT_SWIPING; + /** @private {!SWIPING_STATE_ENUM} */ + this.swipingState_ = SWIPING_STATE_ENUM.NOT_SWIPING; /** @private {?ConfigDef} */ this.playerConfig_ = null; @@ -479,19 +480,30 @@ export class AmpStoryPlayer { */ initializeButton_() { const option = this.element_.getAttribute('exit-control'); - if (!Object.values(DEPRECATED_BUTTON_TYPES).includes(option)) { + if (!isEnumValue(DEPRECATED_BUTTON_TYPES_ENUM, option)) { return; } const button = this.doc_.createElement('button'); this.rootEl_.appendChild(button); - button.classList.add(DEPRECATED_BUTTON_CLASSES[option]); - button.classList.add(DEPRECATED_BUTTON_CLASSES.BASE); + const isBack = option === DEPRECATED_BUTTON_TYPES_ENUM.BACK; + button.classList.add( + isBack + ? DEPRECATED_BUTTON_CLASSES_ENUM.BACK + : DEPRECATED_BUTTON_CLASSES_ENUM.CLOSE + ); + button.classList.add(DEPRECATED_BUTTON_CLASSES_ENUM.BASE); button.addEventListener('click', () => { this.element_.dispatchEvent( - createCustomEvent(this.win_, DEPRECATED_EVENT_NAMES[option], dict({})) + createCustomEvent( + this.win_, + isBack + ? DEPRECATED_EVENT_NAMES_ENUM.BACK + : DEPRECATED_EVENT_NAMES_ENUM.CLOSE, + dict({}) + ) ); }); } @@ -609,24 +621,26 @@ export class AmpStoryPlayer { messaging.sendRequest( 'onDocumentState', - dict({'state': STORY_MESSAGE_STATE_TYPE.PAGE_ATTACHMENT_STATE}), + dict({ + 'state': STORY_MESSAGE_STATE_TYPE_ENUM.PAGE_ATTACHMENT_STATE, + }), false ); messaging.sendRequest( 'onDocumentState', - dict({'state': STORY_MESSAGE_STATE_TYPE.CURRENT_PAGE_ID}), + dict({'state': STORY_MESSAGE_STATE_TYPE_ENUM.CURRENT_PAGE_ID}), false ); messaging.sendRequest( 'onDocumentState', - dict({'state': STORY_MESSAGE_STATE_TYPE.MUTED_STATE}) + dict({'state': STORY_MESSAGE_STATE_TYPE_ENUM.MUTED_STATE}) ); messaging.sendRequest( 'onDocumentState', - dict({'state': STORY_MESSAGE_STATE_TYPE.UI_STATE}) + dict({'state': STORY_MESSAGE_STATE_TYPE_ENUM.UI_STATE}) ); messaging.registerHandler('documentStateUpdate', (event, data) => { @@ -699,17 +713,17 @@ export class AmpStoryPlayer { * @private */ initializeLoadingListeners_(iframeEl) { - this.rootEl_.classList.add(LoadStateClass.LOADING); + this.rootEl_.classList.add(LOAD_STATE_CLASS_ENUM.LOADING); iframeEl.onload = () => { - this.rootEl_.classList.remove(LoadStateClass.LOADING); - this.rootEl_.classList.add(LoadStateClass.LOADED); - this.element_.classList.add(LoadStateClass.LOADED); + this.rootEl_.classList.remove(LOAD_STATE_CLASS_ENUM.LOADING); + this.rootEl_.classList.add(LOAD_STATE_CLASS_ENUM.LOADED); + this.element_.classList.add(LOAD_STATE_CLASS_ENUM.LOADED); }; iframeEl.onerror = () => { - this.rootEl_.classList.remove(LoadStateClass.LOADING); - this.rootEl_.classList.add(LoadStateClass.ERROR); - this.element_.classList.add(LoadStateClass.ERROR); + this.rootEl_.classList.remove(LOAD_STATE_CLASS_ENUM.LOADING); + this.rootEl_.classList.add(LOAD_STATE_CLASS_ENUM.ERROR); + this.element_.classList.add(LOAD_STATE_CLASS_ENUM.ERROR); }; } @@ -904,7 +918,7 @@ export class AmpStoryPlayer { */ getStoryState(storyStateType) { switch (storyStateType) { - case STORY_STATE_TYPE.PAGE_ATTACHMENT_STATE: + case STORY_STATE_TYPE_ENUM.PAGE_ATTACHMENT_STATE: this.getPageAttachmentState_(); break; default: @@ -960,7 +974,7 @@ export class AmpStoryPlayer { messaging .sendRequest( 'getDocumentState', - {state: STORY_MESSAGE_STATE_TYPE.UI_STATE}, + {state: STORY_MESSAGE_STATE_TYPE_ENUM.UI_STATE}, true ) .then((event) => resolve(event.value)); @@ -1137,10 +1151,10 @@ export class AmpStoryPlayer { updatePosition_(story) { const position = story.distance === 0 - ? StoryPosition.CURRENT + ? STORY_POSITION_ENUM.CURRENT : story.idx > this.currentIdx_ - ? StoryPosition.NEXT - : StoryPosition.PREVIOUS; + ? STORY_POSITION_ENUM.NEXT + : STORY_POSITION_ENUM.PREVIOUS; requestAnimationFrame(() => { const {iframe} = story; @@ -1168,7 +1182,7 @@ export class AmpStoryPlayer { if (this.currentStoryLoadDeferred_) { // Cancel previous story load promise. this.currentStoryLoadDeferred_.reject( - `[${LOG_TYPE.DEV}] Cancelling previous story load promise.` + `[${LOG_TYPE_ENUM.DEV}] Cancelling previous story load promise.` ); } @@ -1241,7 +1255,7 @@ export class AmpStoryPlayer { } }) .catch((err) => { - if (err.includes(LOG_TYPE.DEV)) { + if (err.includes(LOG_TYPE_ENUM.DEV)) { return; } console /*OK*/ @@ -1416,7 +1430,7 @@ export class AmpStoryPlayer { updateMutedState_(story, mutedValue) { this.updateStoryState_( story, - STORY_MESSAGE_STATE_TYPE.MUTED_STATE, + STORY_MESSAGE_STATE_TYPE_ENUM.MUTED_STATE, mutedValue ); } @@ -1432,7 +1446,7 @@ export class AmpStoryPlayer { messaging .sendRequest( 'getDocumentState', - {state: STORY_MESSAGE_STATE_TYPE.PAGE_ATTACHMENT_STATE}, + {state: STORY_MESSAGE_STATE_TYPE_ENUM.PAGE_ATTACHMENT_STATE}, true ) .then((event) => this.dispatchPageAttachmentEvent_(event.value)); @@ -1528,19 +1542,19 @@ export class AmpStoryPlayer { */ onDocumentStateUpdate_(data, messaging) { switch (data.state) { - case STORY_MESSAGE_STATE_TYPE.PAGE_ATTACHMENT_STATE: + case STORY_MESSAGE_STATE_TYPE_ENUM.PAGE_ATTACHMENT_STATE: this.onPageAttachmentStateUpdate_(/** @type {boolean} */ (data.value)); break; - case STORY_MESSAGE_STATE_TYPE.CURRENT_PAGE_ID: + case STORY_MESSAGE_STATE_TYPE_ENUM.CURRENT_PAGE_ID: this.onCurrentPageIdUpdate_( /** @type {string} */ (data.value), messaging ); break; - case STORY_MESSAGE_STATE_TYPE.MUTED_STATE: + case STORY_MESSAGE_STATE_TYPE_ENUM.MUTED_STATE: this.onMutedStateUpdate_(/** @type {string} */ (data.value)); break; - case STORY_MESSAGE_STATE_TYPE.UI_STATE: + case STORY_MESSAGE_STATE_TYPE_ENUM.UI_STATE: // Handles UI state updates on window resize. this.onUiStateUpdate_(/** @type {number} */ (data.value)); break; @@ -1592,7 +1606,7 @@ export class AmpStoryPlayer { messaging .sendRequest( 'getDocumentState', - dict({'state': STORY_MESSAGE_STATE_TYPE.STORY_PROGRESS}), + dict({'state': STORY_MESSAGE_STATE_TYPE_ENUM.STORY_PROGRESS}), true ) .then((progress) => { @@ -1634,8 +1648,8 @@ export class AmpStoryPlayer { } isVisible - ? button.classList.remove(DEPRECATED_BUTTON_CLASSES.HIDDEN) - : button.classList.add(DEPRECATED_BUTTON_CLASSES.HIDDEN); + ? button.classList.remove(DEPRECATED_BUTTON_CLASSES_ENUM.HIDDEN) + : button.classList.add(DEPRECATED_BUTTON_CLASSES_ENUM.HIDDEN); } /** @@ -1795,7 +1809,7 @@ export class AmpStoryPlayer { this.touchEventState_.startY = 0; this.touchEventState_.lastX = 0; this.touchEventState_.isSwipeX = null; - this.swipingState_ = SwipingState.NOT_SWIPING; + this.swipingState_ = SWIPING_STATE_ENUM.NOT_SWIPING; } /** @@ -1812,14 +1826,14 @@ export class AmpStoryPlayer { if (gesture.last === true) { const delta = Math.abs(deltaX); - if (this.swipingState_ === SwipingState.SWIPING_TO_LEFT) { + if (this.swipingState_ === SWIPING_STATE_ENUM.SWIPING_TO_LEFT) { delta > TOGGLE_THRESHOLD_PX && (this.getSecondaryStory_() || this.isCircularWrappingEnabled_) ? this.next_() : this.resetStoryStyles_(); } - if (this.swipingState_ === SwipingState.SWIPING_TO_RIGHT) { + if (this.swipingState_ === SWIPING_STATE_ENUM.SWIPING_TO_RIGHT) { delta > TOGGLE_THRESHOLD_PX && (this.getSecondaryStory_() || this.isCircularWrappingEnabled_) ? this.previous_() @@ -1861,7 +1875,7 @@ export class AmpStoryPlayer { */ getSecondaryStory_() { const nextStoryIdx = - this.swipingState_ === SwipingState.SWIPING_TO_LEFT + this.swipingState_ === SWIPING_STATE_ENUM.SWIPING_TO_LEFT ? this.currentIdx_ + 1 : this.currentIdx_ - 1; @@ -1956,10 +1970,10 @@ export class AmpStoryPlayer { let secondaryTranslate; if (deltaX < 0) { - this.swipingState_ = SwipingState.SWIPING_TO_LEFT; + this.swipingState_ = SWIPING_STATE_ENUM.SWIPING_TO_LEFT; secondaryTranslate = `translate3d(calc(100% + ${deltaX}px), 0, 0)`; } else { - this.swipingState_ = SwipingState.SWIPING_TO_RIGHT; + this.swipingState_ = SWIPING_STATE_ENUM.SWIPING_TO_RIGHT; secondaryTranslate = `translate3d(calc(${deltaX}px - 100%), 0, 0)`; }