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

Add function statics and object callable properties #55

Merged
merged 8 commits into from
Jun 5, 2019

Conversation

wgao19
Copy link
Contributor

@wgao19 wgao19 commented Jun 3, 2019

What does this PR do?

Adds function statics that is present in Flow.

Anything the maintainer needs to take note?

The example in this addition works on 0.99 due to some fixes. Although callable property has been around Flow since 0.75 (Store object call properties in internal slot , Add support for [[call]] internal slot properties ), and its precursor syntax (already deprecated in 0.75 and removed in 0.99) was present at least before 0.47.

@tanhauhau
Copy link

is callable property documented anywhere in https://flow.org/en/docs/?

@wgao19
Copy link
Contributor Author

wgao19 commented Jun 3, 2019

Not really, created an issue facebook/flow#7777.

@goodmind
Copy link

goodmind commented Jun 3, 2019

I think TS supports this?

interface Foo {
  (foo: number): number;
  cache: {foo: string}
}

Same for Flow, so not sure why new syntax was added

@niieani
Copy link
Owner

niieani commented Jun 3, 2019

Yeah, I think @goodmind is right, TypeScript supports this as per example. If you want to update the PR I'm be happy to merge.

README.md Outdated
cache: {
[number]: number,
},
[[call]](number): number, // callable property

Choose a reason for hiding this comment

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

I think [[call]]: (number) => number is a more commonly used syntax. Flow also supports {(number) => number} Maybe add examples for all syntaxes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose you are referring to the $call property syntax?

type MemoizedFactorialType = {
  cache: {
    [number]: number,
  },
  (number) => number, // is a syntax error
}

Try Flow

The $call property syntax is deprecated and removed therefore I think we should leave that out.

Choose a reason for hiding this comment

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

I suppose you are referring to the $call property syntax?

type MemoizedFactorialType = {
  cache: {
    [number]: number,
  },
  (number) => number, // is a syntax error
}

Try Flow

The $call property syntax is deprecated and removed therefore I think we should leave that out.

Sorry, I should've been more clear. What I meant was include all the "syntaxes":

  1. Your original example: { [[call]](number): number } (explicit call property)
  2. The more common format of the above: { [[call]]: (number) => number } (explicit call property)
  3. Implicit call property: { (number): number } (implicit call property).

The reason why I suggested the second example is because I think it's more common in libdefs (maybe I am wrong), too. Also, I think it might be the easiest to read/notice from the three, but maybe that's just my bias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Sorry about misreading the last comment. Will work on integrating them.

- Use simpler examples
- Add TypeScript on object callables
@wgao19
Copy link
Contributor Author

wgao19 commented Jun 3, 2019

Thank you all for helping!

I've updated the branch with:

  • user simpler examples on callable objects and
  • added TypeScript on callable objects

I'm not very familiar with TypeScript. For my second example with overloaded functions, my TypeScript playground isn't reporting error whereas Try Flow works as expected. Will anybody please help me out here? I'll update in the branch then the callable property part should be good to go.

@goodmind I guess the TC39 proposal was about function statics? It's probably not that natural to look at it the other way around when coming in from one side of the problem.

@tanhauhau
Copy link

tanhauhau commented Jun 3, 2019

Shouldn't the TypeScript playground be like this ?

@wgao19
Copy link
Contributor Author

wgao19 commented Jun 3, 2019

Thanks @tanhauhau and updated.

In this case, the object callable looks quite the same across Flow and TypeScript. @niieani does this count as "same syntax"?

README.md Outdated
Multiple call properties are supported

```js
const foo: { (): string, (x: number): string } = function(x?: number): string {
Copy link

@omninonsense omninonsense Jun 3, 2019

Choose a reason for hiding this comment

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

I would be careful with this example. Here it might work, but this is not what actually happens at the type-level, and it might be confusing with real-world examples.

Flow will create multiple call signatures and intersect them, so the type of { (): string, (x: number): string } is actually {(): string} & {(number): string}, but we can simplify it to (() => string) & ((number) => string) for the sake of the argument here.

Flow doesn't support dependent types, so this can create a situation where we can describe overloaded implementations for functions that cannot be implemented using a single function body (from flow's perspective). Take this example:

type F = {
  (): string,
  (number): string,
  (string): number
}

const f: F = function(x?: number | string) {
	if (typeof x === 'number') return ''
  	else return 0
}

We could change the interface to be (number | string) => (number | string), but then the function consumer (which could be someone using your library) has to litter their code with useless checks (or flow-only code comments).

Edit: typos and fluff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I see. Thanks for sharing this! It’s a new perspective for me indeed, really appreciate it! 🧡💙🧡💙

Do you have a link to any real world example, e.g., a library you work on, so that I can see this in action and understand it more comprehensively?

Also, do you have links to any “further reading” that we can throw in as reference?

Copy link

@omninonsense omninonsense Jun 3, 2019

Choose a reason for hiding this comment

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

Hmm I see. Thanks for sharing this! It’s a new perspective for me indeed, really appreciate it!

No problem! These things are sadly not that well documented. I'm really glad you're putting effort into documenting this!

Do you have a link to any real world example, e.g., a library you work on, so that I can see this in action and understand it more comprehensively?

A few examples from the top of my head:

  • immer.js uses it to overload the produce (default export) function which has multiple call signatures
  • styled-components uses it to allow being called with a string (react intrinsics), and to allow wrapping of components
  • react-redux's connect relies on overloading, too, depending on which arguments got passed
  • re-reselect uses it to overload the createCachedSelector (also re-select).

Also, do you have links to any “further reading” that we can throw in as reference?

I can't think of anything right now. Most of my flow knowledge is kind of accumulated over time from keeping up with releases, understanding the flow code, and reading the commit messages.

Other than that, I think you're doing a great job of creating "further reading" materials yourself! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @omninonsense,

I've tried the example above and I could not make it work. Both Try Flow and TypeScript Playground are reporting roughly the same errors. It seems that they both are unable to consider the return type against their respective branches.

I've included a couple of simpler examples that exemplify the different syntax, as mentioned in your other comment.

Does it look good to you?

Thanks again for your inputs!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow, thanks for sharing the libraries! Such a great learning material (massive chunks of overloaded call properties in re-select 😲

I see in styled-components libdef it does have the use case for overloaded branches with separate returns. Any idea why it's not working with our primitive examples?

declare export default StyledComponentList & {
  [[call]]: <S : string>(S) => $ElementType<StyledComponentList, S>,
  [[call]]: <P : {}, C : React$ComponentType<P>>(C) => StyledComponentType<C>
};

Choose a reason for hiding this comment

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

I've tried the example above and I could not make it work.

This is exactly why I added the example. 🙂 It's not possible to write overloaded function bodies. The best we can do is something like this:

declare function f(): string;
declare function f(number): string;
declare function f(string): number;

function f(n) {
    if (typeof f === 'undefined') return ''
    if (typeof f === 'number') return ''
    if (typeof f === 'string') return 0
}

const a = f()
const b = f(0)
const c = f('')
const d = f(null) // error, we haven't described f(null)

This is not great since, but given the dynamic nature of javascript, this is probably the best it can get. We need to remember that Flow is just a superset of JS, not a compiler.

Choose a reason for hiding this comment

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

The reason your example in the documentation worked is only because of the simple nature of the example and the fact that all signatures are compatible (since (?number => number) can be described as (() => number) & ((number) => number), which is what's happening here), but it might mislead someone reading the example that flow supports overloaded function implementations (when it only supports overloaded function declarations and overloaded call signatures).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see 😀 Thanks for all the sharing, that's enlightening!
I guess we can get this merged now :)))))

Choose a reason for hiding this comment

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

Regarding styled components: flow is smart enough to select the right overload declaration (at the type level) at call time based on arguments. It just doesn't have a way of writing overload implementations (because this is something JavaScript can't do either). The reason it supports overloaded declarations is because it makes it easier to describe APIs that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Library definitions are a whole new field for me. Thanks for the pointer :) That was really helpful for me to understand the libdefs I'm using for Flow.

README.md Outdated
cache: {
[number]: number,
},
[[call]](number): number, // callable property

Choose a reason for hiding this comment

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

I suppose you are referring to the $call property syntax?

type MemoizedFactorialType = {
  cache: {
    [number]: number,
  },
  (number) => number, // is a syntax error
}

Try Flow

The $call property syntax is deprecated and removed therefore I think we should leave that out.

Sorry, I should've been more clear. What I meant was include all the "syntaxes":

  1. Your original example: { [[call]](number): number } (explicit call property)
  2. The more common format of the above: { [[call]]: (number) => number } (explicit call property)
  3. Implicit call property: { (number): number } (implicit call property).

The reason why I suggested the second example is because I think it's more common in libdefs (maybe I am wrong), too. Also, I think it might be the easiest to read/notice from the three, but maybe that's just my bias.

@niieani
Copy link
Owner

niieani commented Jun 3, 2019

I'd move it to same syntax, with an additional note about the Flow-specific syntax: [[call]]. Thanks all for contributing with the reviews/comments!

@goodmind
Copy link

goodmind commented Jun 4, 2019

Please update latest example, TypeScript doesn't support (number) => string syntax, it should be (value: number) => string

Typescript thinks it is (number: any) => string

@wgao19
Copy link
Contributor Author

wgao19 commented Jun 4, 2019

Thank you @goodmind, updated. Does it look good to you?

@goodmind
Copy link

goodmind commented Jun 5, 2019

Yeah, it's OK. Maybe there should be separate section about (number) => string syntax, to not confuse anyone. I don't think it is documented in Flow docs either
Nice work! 👍

@niieani
Copy link
Owner

niieani commented Jun 5, 2019

Thanks everyone for contributing! Let's open another PR about (Type) => Type syntax.

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.

5 participants