Moved typings into repo, and updated typings to improve type safety. by dmiller9911 · Pull Request #302 · Khan/aphrodite

dmiller9911 · 2018-02-21T23:25:24Z

Replaces #258 since the contributor has asked for someone else to take over.

Provides intellisense for propteries in StyleSheet.Create
Provides type safety for fonts
updated to include new minify function and StyleSheetTestUtils.getBufferedStyles
Removed dependency on React.CSSProperties typings by pulling in typings from glamorous, and editing to match Aphrodite.

jlfwong · 2018-02-22T02:39:18Z

This is awesome ✨!

I'll give this a try tonight by using it a TypeScript project I have. Thanks for taking this on :)

jlfwong

This is great! I just tried it in a project of mine and ran into no problems. A few questions inline, then this should be good to land.

jlfwong · 2018-02-22T05:17:52Z

+
+export var StyleSheet: StyleSheetStatic;
+
+type CSSInputTypes = StyleDeclarationValue | false | null | void;


You are correct. Nice find. I moved this directly from the other PR.

I actually had to put null back in. While doing tests null was not explicitly allowed, and failed the css(null) test

jlfwong · 2018-02-22T05:19:51Z

+ */
+export type StyleDeclarationMap = Map<keyof CSSProperties, string | number>;
+export type StyleDeclaration<T = {}> = {
+    [P in keyof T]: CSSProperties | StyleDeclarationMap


I didn't realize that aphrodite now explicitly supports calls like:

const declMap = new Map() declMap.set("background", "red"); StyleSheet.create({ foo: declMap })

Does that seem right to you, @lencioni?

Not sure when this was added, but Map support was added to allow key ordering to be honored. https://github.com/Khan/aphrodite#object-key-ordering

Yep, this was added mostly to allow developers to ensure the order of styles in older versions of node. We needed it at Airbnb when it was introduced, but since then we have been able to update our node version to a new enough version that preserves key order in objects so we no longer need this feature. In practice, I'm not sure how many people are using it anymore.

jlfwong · 2018-02-22T05:20:23Z

+export interface StyleDeclarationValue<
+    T extends CSSProperties | StyleDeclarationMap = {}
+> {
+    _len: number;


Since this is part of the private API, I don't know if it's valuable to export these properties, since this is intentionally not part of the public API.

Will remove. Do you think there is value in leaving it as something like this:

export type StyleDeclarationValue = object; ... interface StyleSheetStatic { /** * Create style sheet */ create<T extends StyleDeclaration<T>>( styles: T ): {[K in keyof T]: StyleDeclarationValue }; ... type CSSInputTypes = StyleDeclarationValue | false | void; /** * Get class names from passed styles */ export function css(...styles: CSSInputTypes[]): string;

This will allow people to import the return value type for StyleSheet.create incase they are passing the objects around before using css. It doesn't afford a ton of type safety, but it will not leak any private API members.

That sounds good to me :)

jlfwong · 2018-02-22T05:22:33Z

+export var StyleSheetTestUtils: StyleSheetTestUtilsStatic;
+
+export interface SelectorHandler {
+    (selector: string, baseSelector: string, callback: (selector: string) => string):


Am I misread this line? This looks like a function with a return type of | string | null (with the leading |) Is this supposed to be just string | null?

yeah, just string | null prettier adds the leading | for multiline statements like this. I assume to make it more readable. If this is a pattern you think should not be here I can remove. I ran all the d.ts files through prettier before adding them.

jlfwong · 2018-02-22T05:26:20Z

I'm also unsure whether it's kosher to just link to the GitHub page for glamorous, or if we need to include the full MIT license in that file.

dmiller9911 · 2018-02-22T20:06:08Z

Yeah, I was unsure about the link to glamorous. I followed their lead with it. Their typing file has a link to the react typings since they use that as a base. I also made some changes to the file to better align with Aphrodite. @lencioni any idea on the best way to handle that?

lencioni · 2018-02-22T22:56:56Z

I don't think it would hurt to include the license in the file, but I have no idea if it is necessary. I bet @ljharb would know though.

lencioni · 2018-02-22T23:01:01Z

@dmiller9911 is it possible to have something run in CI that ensures that these types (or as many as possible) are always accurate?

jlfwong · 2018-02-23T00:05:46Z

@dmiller9911 I think will be good to merge after the update to remove references to private fields!

dmiller9911 · 2018-02-26T15:00:45Z

@lencioni that is a good idea. Should not be too difficult. I will get something up today or tomorrow. Along with the changes suggested by @jlfwong

Replaces Khan#258 since the contributor has asked for someone else to take over. * Provides intellisense for propteries in StyleSheet.Create * Provides type safety for fonts * updated to include new minify function and StyleSheetTestUtils.getBufferedStyles * Removed dependency on React.CSSProperties typings by pulling in typings from glamorous, and editing to match Aphrodite.

…ed exports from css-properties. Add license/permission from glamorous typings to be safe. Add tests to verify typing functionality over time.

jlfwong · 2018-03-02T21:32:25Z

This is great @dmiller9911! Thanks for putting this together and responding gracefully to feedback :)

lencioni requested a review from jlfwong February 21, 2018 23:45

jlfwong approved these changes Feb 22, 2018

View reviewed changes

dmiller9911 force-pushed the typings branch from 624e6af to eec853e Compare March 2, 2018 20:45

Remove StyleDeclarationValue private members from export. Remove unus…

6abe537

…ed exports from css-properties. Add license/permission from glamorous typings to be safe. Add tests to verify typing functionality over time.

dmiller9911 force-pushed the typings branch from eec853e to 6abe537 Compare March 2, 2018 21:03

jlfwong merged commit c24225a into Khan:master Mar 2, 2018

lencioni mentioned this pull request Mar 4, 2018

Updated Typescript definitions and included in package #258

Closed

lencioni mentioned this pull request Mar 15, 2018

Add extend to the type definition file #172

Closed

This was referenced Mar 16, 2018

Bump aphrodite from 1.2.5 to 2.1.0 in /frontend uclapi/uclapi#385

Closed

Bump aphrodite from 1.2.5 to 2.1.0 tmhn/react#89

Closed

ghost mentioned this pull request Mar 19, 2018

Update aphrodite in / from 1.2.5 to 2.1.0 severest/retrobot#67

Merged


		export var StyleSheet: StyleSheetStatic;

		type CSSInputTypes = StyleDeclarationValue \| false \| null \| void;

Conversation

dmiller9911 commented Feb 21, 2018

Uh oh!

jlfwong commented Feb 22, 2018

Uh oh!

jlfwong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlfwong Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlfwong commented Feb 22, 2018

Uh oh!

dmiller9911 commented Feb 22, 2018

Uh oh!

lencioni commented Feb 22, 2018

Uh oh!

lencioni commented Feb 22, 2018

Uh oh!

jlfwong commented Feb 23, 2018

Uh oh!

dmiller9911 commented Feb 26, 2018

Uh oh!

jlfwong commented Mar 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jlfwong Feb 22, 2018 •

edited

Loading