Skip to content

Commit

Permalink
use tsc typechecking for carousel 0.1 (#36296)
Browse files Browse the repository at this point in the history
* ts-carousel 0.1

* split base-element to its own file

* rebase + add carousel to check-types

* fixes

* type auxuliary carousel funcs

* address ryan feedback
  • Loading branch information
samouri committed Oct 12, 2021
1 parent c44857a commit a4b8baf
Show file tree
Hide file tree
Showing 7 changed files with 255 additions and 36 deletions.
18 changes: 7 additions & 11 deletions build-system/tasks/check-types.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const argv = require('minimist')(process.argv.slice(2));
const fastGlob = require('fast-glob');
const path = require('path');
const {
createCtrlcHandler,
exitCtrlcHandler,
Expand Down Expand Up @@ -42,12 +41,11 @@ const getExtensionSrcPaths = () =>
/**
* Object of targets to check with TypeScript.
*
* @type {Object<string, {tsconfig: string}>}
* @type {Object<string, string>}
*/
const TSC_TYPECHECK_TARGETS = {
'compiler': {
tsconfig: 'src/compiler/tsconfig.json',
},
'compiler': 'src/compiler',
'carousel': 'extensions/amp-carousel/0.1',
};

/**
Expand Down Expand Up @@ -112,7 +110,7 @@ const CLOSURE_TYPE_CHECK_TARGETS = {
// errors.
'low-bar': {
entryPoints: ['src/amp.js'],
extraGlobs: ['{src,extensions}/**/*.js', getLowBarExclusions()],
extraGlobs: ['{src,extensions}/**/*.js', ...getLowBarExclusions()],
onError(msg) {
const lowBarErrors = [
'JSC_BAD_JSDOC_ANNOTATION',
Expand Down Expand Up @@ -201,7 +199,7 @@ async function typeCheck(targetName) {
*/
async function tscTypeCheck(targetName) {
execOrThrow(
`npx -p typescript tsc --project ${TSC_TYPECHECK_TARGETS[targetName].tsconfig}`,
`npx -p typescript tsc --project ${TSC_TYPECHECK_TARGETS[targetName]}/tsconfig.json`,
`Type checking ${targetName} failed`
);
log(green('SUCCESS:'), 'Type-checking passed for target', cyan(targetName));
Expand All @@ -211,12 +209,10 @@ async function tscTypeCheck(targetName) {
* Returns the exclusion glob for telling closure to ignore all paths
* being checked via TS.
*
* @return {string}
* @return {string[]}
*/
function getLowBarExclusions() {
return Object.values(TSC_TYPECHECK_TARGETS)
.map((target) => '!' + path.dirname(target.tsconfig))
.join(',');
return Object.values(TSC_TYPECHECK_TARGETS).map((dir) => `!${dir}`);
}

/**
Expand Down
29 changes: 8 additions & 21 deletions extensions/amp-carousel/0.1/base-carousel.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {Keys} from '#core/constants/key-codes';
import {Services} from '#service';
import {isAmp4Email} from '#core/document/format';
import {observeIntersections} from '#core/dom/layout/viewport-observer';
import {toggleAttribute} from '#core/dom';

const _CONTROL_HIDE_ATTRIBUTE = 'i-amphtml-carousel-hide-buttons';
Expand All @@ -19,17 +18,14 @@ export class BaseCarousel extends AMP.BaseElement {
constructor(element) {
super(element);

/** @private {?Element} */
/** @private {?HTMLDivElement} */
this.prevButton_ = null;

/** @private {?Element} */
/** @private {?HTMLDivElement} */
this.nextButton_ = null;

/** @private {boolean} */
this.showControls_ = false;

/** @private {?UnlistenDef} */
this.unobserveIntersections_ = null;
}

/** @override */
Expand Down Expand Up @@ -73,8 +69,8 @@ export class BaseCarousel extends AMP.BaseElement {
/**
* Builds a carousel button for next/prev.
* @param {string} className
* @param {function()} onInteraction
* @return {?Element}
* @param {function():void} onInteraction
* @return {?HTMLDivElement}
*/
buildButton(className, onInteraction) {
const button = this.element.ownerDocument.createElement('div');
Expand Down Expand Up @@ -168,9 +164,9 @@ export class BaseCarousel extends AMP.BaseElement {
*/
setControlsState() {
this.prevButton_.classList.toggle('amp-disabled', !this.hasPrev());
this.prevButton_.setAttribute('aria-disabled', !this.hasPrev());
this.prevButton_.setAttribute('aria-disabled', String(!this.hasPrev()));
this.nextButton_.classList.toggle('amp-disabled', !this.hasNext());
this.nextButton_.setAttribute('aria-disabled', !this.hasNext());
this.nextButton_.setAttribute('aria-disabled', String(!this.hasNext()));
this.prevButton_.tabIndex = this.hasPrev() ? 0 : -1;
this.nextButton_.tabIndex = this.hasNext() ? 0 : -1;
}
Expand Down Expand Up @@ -229,19 +225,8 @@ export class BaseCarousel extends AMP.BaseElement {
);
}

/** @override */
layoutCallback() {
this.unobserveIntersections_ = observeIntersections(
this.element,
({isIntersecting}) => this.viewportCallback(isIntersecting)
);
return Promise.resolve();
}

/** @override */
unlayoutCallback() {
this.unobserveIntersections_?.();
this.unobserveIntersections_ = null;
return true;
}

Expand All @@ -250,13 +235,15 @@ export class BaseCarousel extends AMP.BaseElement {
*/
hasPrev() {
// Subclasses may override.
return false;
}

/**
* @return {boolean}
*/
hasNext() {
// Subclasses may override.
return false;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-carousel/0.1/scrollable-carousel.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ export class AmpScrollableCarousel extends BaseCarousel {
* Escapes Left and Right arrow key events on the carousel container.
* This is to prevent them from doubly interacting with surrounding viewer
* contexts such as email clients when interacting with the amp-carousel.
* @param {!Event} event
* @param {!KeyboardEvent} event
* @private
*/
keydownHandler_(event) {
Expand Down Expand Up @@ -304,7 +304,7 @@ export class AmpScrollableCarousel extends BaseCarousel {

/**
* @param {number} pos
* @param {function(!Element)} callback
* @param {function(!Element):void} callback
* @private
*/
withinWindow_(pos, callback) {
Expand Down
85 changes: 85 additions & 0 deletions extensions/amp-carousel/0.1/shame.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
declare module '#utils/log' {
export var dev: any;
export var user: any;
export var userAssert: any;
}

declare module '#utils/animation' {
export var Animation: any;
}

declare module '#utils/event-helper' {
export var createCustomEvent: any;
export var listen: any;
}

declare module '#experiments' {
export var dict: any;
export var isExperimentOn: any;
}

declare module '#service' {
export var Services: any;
}
declare module '#utils/analytics' {
export var triggerAnalyticsEvent: any;
}

// TODO: everything below are core stubs, which we can remove once Core has been
// converted to TS.
declare module '#core/constants/key-codes' {
export var Keys: any;
}

declare module '#core/document/format' {
export var isAmp4Email: any;
}

declare module '#core/dom/layout/viewport-observer' {
export var observeIntersections: any;
}

declare module '#core/dom' {
export var toggleAttribute: any;
export var dispatchCustomEvent: any;
}

declare module '#core/dom/query' {
export var realChildElements: any;
export var closestAncestorElementBySelector: any;
}

declare module '#core/constants/action-constants' {
export var ActionTrust: any;
}

declare module '#core/dom/layout' {
export var isLayoutSizeFixed: any;
export var isLayoutSizeDefined: any;
}

declare module '#core/dom/layout/size-observer' {
export var observeContentSize: any;
export var unobserveContentSize: any;
}

declare module '#core/types' {
export var isFiniteNumber: any;
}

declare module '#core/types/object' {
export var dict: any;
}

declare module '#core/dom/transition' {
export var numeric: any;
}

declare module '#core/dom/style' {
export var getStyle: any;
export var setStyle: any;
}

declare module '#core/data-structures/curve' {
export var bezierCurve: any;
}
4 changes: 2 additions & 2 deletions extensions/amp-carousel/0.1/slidescroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ export class AmpSlideScroll extends BaseCarousel {
}

/**
* @param {!../layout-rect.LayoutSizeDef} size
* @param {!LayoutSize} size
* @private
*/
onResized_(size) {
Expand Down Expand Up @@ -531,7 +531,7 @@ export class AmpSlideScroll extends BaseCarousel {
* Escapes Left and Right arrow key events on the carousel container.
* This is to prevent them from doubly interacting with surrounding viewer
* contexts such as email clients when interacting with the amp-carousel.
* @param {!Event} event
* @param {!KeyboardEvent} event
* @private
*/
keydownHandler_(event) {
Expand Down
10 changes: 10 additions & 0 deletions extensions/amp-carousel/0.1/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"compilerOptions": {
"allowJs": true,
"checkJs": true,
"noEmit": true,
"paths": {}
},
"include": ["*.js", "**/*.d.ts", "../../../src/*.d.ts"],
"exclude": []
}

0 comments on commit a4b8baf

Please sign in to comment.