Skip to content

Commit

Permalink
Lint several static enums (#36782)
Browse files Browse the repository at this point in the history
* Use static enum linting with UrlReplacementPolicy

* Lint already static enums in src

* Fix LOADING_ELEMENTS_ENUMENUM
  • Loading branch information
jridgewell committed Nov 6, 2021
1 parent 70d41cf commit 97dcb28
Show file tree
Hide file tree
Showing 24 changed files with 100 additions and 95 deletions.
4 changes: 2 additions & 2 deletions docs/building-an-amp-extension.md
Original file line number Diff line number Diff line change
Expand Up @@ -516,10 +516,10 @@ Consider showing a loading indicator if your element is expected to take
a long time to load (for example, loading a GIF, video or iframe). AMP
has a built-in mechanism to show a loading indicator simply by
listing your element so it's allowed to show it. You can do that inside the `layout.js`
file in the `LOADING_ELEMENTS_` object.
file in the `LOADING_ELEMENTS_ENABLED` object.

```javascript
export const LOADING_ELEMENTS_ = {
export const LOADING_ELEMENTS_ENABLED = {
...
'AMP-YOUTUBE': true,
'AMP-MY-ELEMENT': true,
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-analytics/0.1/linker-manager.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {AMPDOC_SINGLETON_NAME} from '#core/constants/enums';
import {AMPDOC_SINGLETON_NAME_ENUM} from '#core/constants/enums';
import {ExpansionOptions, variableServiceForDoc} from './variables';
import {Priority} from '#service/navigation';
import {Services} from '#service';
Expand Down Expand Up @@ -214,7 +214,7 @@ export class LinkerManager {
return false;
}

return this.ampdoc_.registerSingleton(AMPDOC_SINGLETON_NAME.LINKER);
return this.ampdoc_.registerSingleton(AMPDOC_SINGLETON_NAME_ENUM.LINKER);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-autocomplete/0.1/amp-autocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {Layout} from '#core/dom/layout';
import {Services} from '#service';
import {SsrTemplateHelper} from '../../../src/ssr-template-helper';
import {
UrlReplacementPolicy,
UrlReplacementPolicy_Enum,
batchFetchJsonFor,
requestForBatchFetch,
} from '../../../src/batched-json';
Expand Down Expand Up @@ -423,7 +423,7 @@ export class AmpAutocomplete extends AMP.BaseElement {
*/
getRemoteData_() {
const ampdoc = this.getAmpDoc();
const policy = UrlReplacementPolicy.ALL;
const policy = UrlReplacementPolicy_Enum.ALL;
const itemsExpr = this.element.getAttribute('items') || 'items';
this.maybeSetSrcFromInput_();
if (this.isSsr_) {
Expand Down
8 changes: 4 additions & 4 deletions extensions/amp-bind/0.1/amp-state.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {createCustomEvent} from '#utils/event-helper';
import {dev, devAssert, userAssert} from '#utils/log';

import {
UrlReplacementPolicy,
UrlReplacementPolicy_Enum,
batchFetchJsonFor,
} from '../../../src/batched-json';
import {getSourceOrigin} from '../../../src/url';
Expand Down Expand Up @@ -144,7 +144,7 @@ export class AmpState extends AMP.BaseElement {
/**
* Wrapper to stub during testing.
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
* @param {!UrlReplacementPolicy} policy
* @param {!UrlReplacementPolicy_Enum} policy
* @param {boolean=} opt_refresh
* @return {!Promise<!JsonObject|!Array<JsonObject>>}
* @private
Expand Down Expand Up @@ -173,8 +173,8 @@ export class AmpState extends AMP.BaseElement {
// by [src] mutation. @see spec/amp-var-substitutions.md
const policy =
isCorsFetch && !isInit
? UrlReplacementPolicy.OPT_IN
: UrlReplacementPolicy.ALL;
? UrlReplacementPolicy_Enum.OPT_IN
: UrlReplacementPolicy_Enum.ALL;

return this.fetch_(ampdoc, policy, opt_refresh).catch((error) => {
const event = error
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-bind/0.1/test/test-amp-state.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {ActionTrust} from '#core/constants/action-constants';

import {Services} from '#service';

import {UrlReplacementPolicy} from '../../../../src/batched-json';
import {UrlReplacementPolicy_Enum} from '../../../../src/batched-json';

describes.realWin(
'AmpState',
Expand Down Expand Up @@ -82,7 +82,7 @@ describes.realWin(
expect(ampState.fetch_).to.have.been.calledOnce;
expect(ampState.fetch_).to.have.been.calledWithExactly(
/* ampdoc */ env.sandbox.match.any,
UrlReplacementPolicy.ALL,
UrlReplacementPolicy_Enum.ALL,
/* refresh */ env.sandbox.match.falsy
);

Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-iframe/0.1/amp-iframe.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {playIgnoringError} from '#core/dom/video';
import {AMPDOC_SINGLETON_NAME} from '#core/constants/enums';
import {AMPDOC_SINGLETON_NAME_ENUM} from '#core/constants/enums';
import {ActionTrust} from '#core/constants/action-constants';
import {IntersectionObserver3pHost} from '#utils/intersection-observer-3p-host';
import {
Expand Down Expand Up @@ -337,7 +337,7 @@ export class AmpIframe extends AMP.BaseElement {
if (this.isTrackingFrame_) {
if (
!this.getAmpDoc().registerSingleton(
AMPDOC_SINGLETON_NAME.TRACKING_IFRAME
AMPDOC_SINGLETON_NAME_ENUM.TRACKING_IFRAME
)
) {
console /*OK*/
Expand Down
8 changes: 4 additions & 4 deletions extensions/amp-list/0.1/amp-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import {LoadMoreService} from './service/load-more-service';

import {CSS} from '../../../build/amp-list-0.1.css';
import {
UrlReplacementPolicy,
UrlReplacementPolicy_Enum,
batchFetchJsonFor,
requestForBatchFetch,
} from '../../../src/batched-json';
Expand Down Expand Up @@ -1620,18 +1620,18 @@ export class AmpList extends AMP.BaseElement {
}

/**
* @return {!UrlReplacementPolicy}
* @return {!UrlReplacementPolicy_Enum}
*/
getPolicy_() {
const src = this.element.getAttribute('src');
// Require opt-in for URL variable replacements on CORS fetches triggered
// by [src] mutation. @see spec/amp-var-substitutions.md
let policy = UrlReplacementPolicy.OPT_IN;
let policy = UrlReplacementPolicy_Enum.OPT_IN;
if (
src == this.initialSrc_ ||
getSourceOrigin(src) == getSourceOrigin(this.getAmpDoc().win.location)
) {
policy = UrlReplacementPolicy.ALL;
policy = UrlReplacementPolicy_Enum.ALL;
}
return policy;
}
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-next-page/0.1/amp-next-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {NextPageService} from './next-page-service';

import {CSS} from '../../../build/amp-next-page-0.1.css';
import {
UrlReplacementPolicy,
UrlReplacementPolicy_Enum,
batchFetchJsonFor,
} from '../../../src/batched-json';
import {getConsentPolicyState} from '../../../src/consent';
Expand Down Expand Up @@ -259,7 +259,7 @@ export class AmpNextPage extends AMP.BaseElement {
*/
fetchConfig_() {
const ampdoc = this.getAmpDoc();
const policy = UrlReplacementPolicy.ALL;
const policy = UrlReplacementPolicy_Enum.ALL;
return batchFetchJsonFor(ampdoc, this.element, {urlReplacement: policy});
}

Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-next-page/1.0/service.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import VisibilityObserver, {ViewportRelativePos} from './visibility-observer';

import {CSS} from '../../../build/amp-next-page-1.0.css';
import {
UrlReplacementPolicy,
UrlReplacementPolicy_Enum,
batchFetchJsonFor,
} from '../../../src/batched-json';
import {
Expand Down Expand Up @@ -927,7 +927,7 @@ export class NextPageService {
this.ampdoc_,
this.getHost_(),
{
urlReplacement: UrlReplacementPolicy.ALL,
urlReplacement: UrlReplacementPolicy_Enum.ALL,
xssiPrefix: this.getHost_().getAttribute('xssi-prefix') || undefined,
}
)
Expand Down
8 changes: 4 additions & 4 deletions extensions/amp-render/1.0/amp-render.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {BaseElement} from './base-element';
import {
BatchFetchOptionsDef,
UrlReplacementPolicy,
UrlReplacementPolicy_Enum,
batchFetchJsonFor,
} from '../../../src/batched-json';
import {Layout} from '#core/dom/layout';
Expand Down Expand Up @@ -132,7 +132,7 @@ export class AmpRender extends BaseElement {
}

/**
* @return {!UrlReplacementPolicy}
* @return {!UrlReplacementPolicy_Enum}
* @private
*/
getPolicy_() {
Expand All @@ -141,12 +141,12 @@ export class AmpRender extends BaseElement {
// by [src] mutation. @see spec/amp-var-substitutions.md
// TODO(dmanek): Update spec/amp-var-substitutions.md with this information
// and add a `Substitution` sections in this component's markdown file.
let policy = UrlReplacementPolicy.OPT_IN;
let policy = UrlReplacementPolicy_Enum.OPT_IN;
if (
src == this.initialSrc_ ||
getSourceOrigin(src) == getSourceOrigin(this.getAmpDoc().win.location)
) {
policy = UrlReplacementPolicy.ALL;
policy = UrlReplacementPolicy_Enum.ALL;
}
return policy;
}
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-render/1.0/test/test-amp-render.js
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ describes.realWin(
expect(text).to.equal('Biden, Joe');
const options = fetchJsonStub.getCall(0).args[2];
expect(options.urlReplacement).to.equal(
BatchedJsonModule.UrlReplacementPolicy.ALL
BatchedJsonModule.UrlReplacementPolicy_Enum.ALL
);
});

Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-viewer-integration/TICKEVENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ As an example if we executed `perf.tick('label')` we assume we have a counterpar
| First contentful paint (since visible) | `fcpv` | First paint with content, offset by first visible time |
| First input delay | `fid` | Millisecond delay in handling the first user input on the page. See https://github.com/WICG/event-timing |
| Cumulative Layout Shift | `cls` | The current maximum layout shift score with 5s windows and a 1s session gap. See https://web.dev/layout-instability-api |
| Cumulative Layout Shift Type Union | `clstu` | The bitwise union of all Element types that caused layout shift (see `ELEMENT_TYPE`) |
| Cumulative Layout Shift Type Union | `clstu` | The bitwise union of all Element types that caused layout shift (see `ELEMENT_TYPE_ENUM`) |
| Cumulative Layout Shift, first exit | `cls-1` | The aggregate layout shift score when the user leaves the page (navigation, tab switching, dismissing application) for the first time. |
| Cumulative Layout Shift, second exit | `cls-2` | The aggregate layout shift score when the user leaves the page (navigation, tab switching, dismissing application) for the second time. |
| Largest Contentful Paint | `lcp` | The time in milliseconds for the largest contentful element to display. |
| Largest Contentful Paint Type | `lcpt` | The LCP target's Element type (see `ELEMENT_TYPE`) |
| Largest Contentful Paint Type | `lcpt` | The LCP target's Element type (see `ELEMENT_TYPE_ENUM`) |
| Largest Contentful Paint (since visible) | `lcpv` | The time in ms for largest contentful element to display, offset by first visible time. Based on render time, falls back to load time. |
| DOM Complete | `domComplete` | Time immediately before the browser sets the current document readiness of the current document to complete |
| DOM Content Loaded Event End | `domContentLoadedEventEnd` | Time immediately after the current document's DOMContentLoaded event completes |
Expand Down
12 changes: 6 additions & 6 deletions src/batched-json.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {assertHttpsUrl} from './url';
*
* @typedef {{
* expr:(string|undefined),
* urlReplacement: (UrlReplacementPolicy|undefined),
* urlReplacement: (UrlReplacementPolicy_Enum|undefined),
* refresh: (boolean|undefined),
* xssiPrefix: (string|undefined),
* }}
Expand All @@ -27,7 +27,7 @@ export let BatchFetchOptionsDef;
/**
* @enum {number}
*/
export const UrlReplacementPolicy = {
export const UrlReplacementPolicy_Enum = {
NONE: 0,
OPT_IN: 1,
ALL: 2,
Expand All @@ -47,7 +47,7 @@ export const UrlReplacementPolicy = {
export function batchFetchJsonFor(ampdoc, element, options = {}) {
const {
expr = '.',
urlReplacement = UrlReplacementPolicy.NONE,
urlReplacement = UrlReplacementPolicy_Enum.NONE,
refresh = false,
xssiPrefix = undefined,
} = options;
Expand All @@ -73,7 +73,7 @@ export function batchFetchJsonFor(ampdoc, element, options = {}) {
* Handles url replacement and constructs the FetchInitJsonDef required for a
* fetch.
* @param {!Element} element
* @param {!UrlReplacementPolicy} replacement If ALL, replaces all URL
* @param {!UrlReplacementPolicy_Enum} replacement If ALL, replaces all URL
* vars. If OPT_IN, replaces allowlisted URL vars. Otherwise, don't expand.
* @param {boolean} refresh Forces refresh of browser cache.
* @return {!Promise<!FetchRequestDef>}
Expand All @@ -84,14 +84,14 @@ export function requestForBatchFetch(element, replacement, refresh) {
// Replace vars in URL if desired.
const urlReplacements = Services.urlReplacementsForDoc(element);
const promise =
replacement >= UrlReplacementPolicy.OPT_IN
replacement >= UrlReplacementPolicy_Enum.OPT_IN
? urlReplacements.expandUrlAsync(url)
: Promise.resolve(url);

return promise.then((xhrUrl) => {
// Throw user error if this element is performing URL substitutions
// without the soon-to-be-required opt-in (#12498).
if (replacement == UrlReplacementPolicy.OPT_IN) {
if (replacement == UrlReplacementPolicy_Enum.OPT_IN) {
const invalid = urlReplacements.collectDisallowedVarsSync(element);
if (invalid.length > 0) {
throw user().createError(
Expand Down
2 changes: 1 addition & 1 deletion src/core/constants/enums.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Registred singleton on AMP doc.
* @enum {number}
*/
export const AMPDOC_SINGLETON_NAME = {
export const AMPDOC_SINGLETON_NAME_ENUM = {
TRACKING_IFRAME: 1,
LINKER: 2,
};
Expand Down
8 changes: 5 additions & 3 deletions src/core/dom/layout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export let DimensionsDef;
* @enum {boolean}
* @private Visible for testing only!
*/
export const LOADING_ELEMENTS_ = {
export const LOADING_ELEMENTS_ENABLED = {
'AMP-AD': true,
'AMP-ANIM': true,
'AMP-EMBED': true,
Expand Down Expand Up @@ -216,13 +216,15 @@ export function getLengthNumeral(length) {
*/
export function isLoadingAllowed(element) {
const tagName = element.tagName.toUpperCase();
return LOADING_ELEMENTS_[tagName] || isIframeVideoPlayerComponent(tagName);
return (
LOADING_ELEMENTS_ENABLED[tagName] || isIframeVideoPlayerComponent(tagName)
);
}

/**
* All video player components must either have a) "video" or b) "player" in
* their name. A few components don't follow this convention for historical
* reasons, so they're present in the LOADING_ELEMENTS_ allowlist.
* reasons, so they're present in the LOADING_ELEMENTS_ENABLED allowlist.
* @param {string} tagName
* @return {boolean}
*/
Expand Down
4 changes: 2 additions & 2 deletions src/service/ampdoc-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ export class AmpDoc {
/** @public @const {!Window} */
this.win = win;

/** @private {!Object<../enums.AMPDOC_SINGLETON_NAME, boolean>} */
/** @private {!Object<../enums.AMPDOC_SINGLETON_NAME_ENUM, boolean>} */
this.registeredSingleton_ = map();

/** @public @const {?AmpDoc} */
Expand Down Expand Up @@ -712,7 +712,7 @@ export class AmpDoc {
/**
* Attempt to register a singleton for each ampdoc.
* Caller need to handle user error when registration returns false.
* @param {!../enums.AMPDOC_SINGLETON_NAME} name
* @param {!../enums.AMPDOC_SINGLETON_NAME_ENUM} name
* @return {boolean}
*/
registerSingleton(name) {
Expand Down
22 changes: 11 additions & 11 deletions src/service/performance-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ let TickEventDef;
/**
* @enum {number}
*/
export const ELEMENT_TYPE = {
export const ELEMENT_TYPE_ENUM = {
other: 0,
image: 1 << 0,
video: 1 << 1,
Expand All @@ -57,33 +57,33 @@ export const ELEMENT_TYPE = {

/**
* @param {?Node} node
* @return {ELEMENT_TYPE}
* @return {ELEMENT_TYPE_ENUM}
*/
function getElementType(node) {
if (node == null) {
return ELEMENT_TYPE.other;
return ELEMENT_TYPE_ENUM.other;
}
const outer = getOutermostAmpElement(node);
const {nodeName} = outer;
if (nodeName === 'IMG' || nodeName === 'AMP-IMG') {
return ELEMENT_TYPE.image;
return ELEMENT_TYPE_ENUM.image;
}
if (nodeName === 'VIDEO' || nodeName === 'AMP-VIDEO') {
return ELEMENT_TYPE.video;
return ELEMENT_TYPE_ENUM.video;
}
if (nodeName === 'AMP-CAROUSEL') {
return ELEMENT_TYPE.carousel;
return ELEMENT_TYPE_ENUM.carousel;
}
if (nodeName === 'AMP-BASE-CAROUSEL') {
return ELEMENT_TYPE.bcarousel;
return ELEMENT_TYPE_ENUM.bcarousel;
}
if (nodeName === 'AMP-AD') {
return ELEMENT_TYPE.ad;
return ELEMENT_TYPE_ENUM.ad;
}
if (!nodeName.startsWith('AMP-') && outer.textContent) {
return ELEMENT_TYPE.text;
return ELEMENT_TYPE_ENUM.text;
}
return ELEMENT_TYPE.other;
return ELEMENT_TYPE_ENUM.other;
}

/**
Expand Down Expand Up @@ -241,7 +241,7 @@ export class Performance {

/**
* Which type of element was chosen as the LCP.
* @private {ELEMENT_TYPE}
* @private {ELEMENT_TYPE_ENUM}
*/
this.largestContentfulPaintType_ = null;

Expand Down

0 comments on commit 97dcb28

Please sign in to comment.