Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

typechecking carousel: remove shame files #37213

Merged
merged 3 commits into from
Jan 6, 2022
Merged

typechecking carousel: remove shame files #37213

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21ef71f
typechecking carousel/compiler: Remove shame files
samouri Dec 14, 2021
10657d8
cleanup, remove accidental shame
samouri Jan 6, 2022
1a687c8
rcebulko comments
samouri Jan 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions extensions/amp-carousel/0.1/build-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ function buildScrollableCarousel(element) {
}

const doc = element.ownerDocument;
const cells = realChildElements(element);
const cells = /** @type {HTMLDivElement[]}*/ (realChildElements(element));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this guaranteed to be DIVs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Technically we aren't even guaranteed they will be HTMLElements..but we currently have code that assumes they are.

I'll cast to HTMLElement for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realChildElements returns an array of the same type as element, so if element is properly typed to HTMLElement, an additional cast shouldn't be necessary

Copy link
Member Author

@samouri samouri Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know that! Very cool. Is it correct though? I think realChildElements can legitimately return Element (i.e. SVG or HTML)

const container = doc.createElement('div');

container.classList.add(ClassNames.SCROLLABLE_CONTAINER);
Expand Down Expand Up @@ -202,7 +202,7 @@ function buildSlideScrollCarousel(element) {
return querySlideScrollCarousel(element);
}
const doc = element.ownerDocument;
const slides = realChildElements(element);
const slides = /** @type {HTMLDivElement[]} */ (realChildElements(element));
element.classList.add(ClassNames.SLIDESCROLL_CAROUSEL);

const slidesContainer = doc.createElement('div');
Expand Down
68 changes: 0 additions & 68 deletions extensions/amp-carousel/0.1/shame.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,71 +24,3 @@ declare module '#service' {
declare module '#utils/analytics' {
export var triggerAnalyticsEvent: any;
}

declare module '#core/dom/css-selectors' {
export var escapeCssSelectorIdent: (s: string) => string;
}

// 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_Enum: 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;
export var isServerRendered: (el: Element) => boolean;
}

declare module '#core/window' {
export var getWin: (element: Element) => Window;
}

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

declare module '#core/constants/action-constants' {
export var ActionTrust_Enum: 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;
}
15 changes: 12 additions & 3 deletions extensions/amp-carousel/0.1/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,17 @@
"allowJs": true,
"checkJs": true,
"noEmit": true,
"paths": {}
"downlevelIteration": true,
"target": "esnext",
"moduleResolution": "node",
"paths": {
"#compiler": ["../../../src/compiler"],
"#compiler/*": ["../../../src/compiler/*"],
"#core": ["../../../src/core"],
"#core/*": ["../../../src/core/*"],
"#third_party/*": ["../../../third_party/*"]
}
},
"include": ["*.js", "**/*.d.ts", "../../../src/*.d.ts"],
"exclude": []
"include": ["*.js", "**/*.d.ts", "../../../src/**/*.d.ts"],
"exclude": ["../../../src/purifier/index.d.ts"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you still extend the base here, but override the noImplicitAny?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll test it out. The thing I'm more concerned about is #paths.

Copy link
Member Author

@samouri samouri Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! We cannot fully do this yet bc of paths.
Not all of #utils is typed yet and carousel utilizes utils.

The rest works!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add .d.ts files for the utils dependency it would make future typechecking progressively easier

}
6 changes: 0 additions & 6 deletions src/base-element.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,6 @@ declare namespace AMP {
) => void;
}

// TODO(samouri): Move the types below to better locations.
declare type AmpElement = HTMLElement;

declare type Layout =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think LayoutSize and LayoutRect can be imported from #core/dom/layout

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sick. I'll do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that didn't work. Cannot import a module from an ambient context :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to import it from within the namespace, though, which is where the types are used (or just import inline on lines 27/28

| 'nodisplay'
| 'fixed'
Expand All @@ -136,6 +133,3 @@ declare type LayoutRect = {
type Mutations = {
[key: string]: null | boolean | string | number | Array<any> | Object;
};

// TODO:(samouri): correctly type this within src/core/types/object/json
declare type JsonObject = any;