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.

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

Type ajax.js and evented.js #4955

Merged
merged 4 commits into from
Jul 10, 2017
Merged

Type ajax.js and evented.js #4955

merged 4 commits into from
Jul 10, 2017

Conversation

jfirebaugh
Copy link
Contributor

No description provided.

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

One question inline

@@ -12,7 +12,7 @@ type UrlObject = {|
params: Array<string>
|};

function makeAPIURL(urlObject: UrlObject, accessToken): string {
function makeAPIURL(urlObject: UrlObject, accessToken?: ?string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why both optional and maybe-string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs (and is able) to accept both null and undefined. It would be clearer to write this as accessToken: string | null | void though -- I'll switch.

* @returns {Object} `this`
*/
setEventedParent(parent, data) {
setEventedParent(parent: ?Evented, data?: Object) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, just saw: should data be Object | () => Object?

@jfirebaugh jfirebaugh merged commit 490f0b1 into master Jul 10, 2017
@jfirebaugh jfirebaugh deleted the more-types branch July 10, 2017 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants