Add type definitions

[mikeal]

quick question: does flow check ignore any places where there aren’t type definitions? if it does, we could add it to pretest and have it as part of the standard test process.

[mikeal]

also, is it possible to put all of these type files in their own directory?

[Gozala]

quick question: does flow check ignore any places where there aren’t type definitions? if it does, we could add it to pretest and have it as part of the standard test process.

It checks only files that start with // @flow however if package does not contain .flowconfig file it will error.

[Gozala]

also, is it possible to put all of these type files in their own directory?

Benefit of this approach is that when flow sees importing say src/block.js it would instead try src/block.js.flow first meaning that dependent modules packages will just be able to use this package as if it was typed.

There is no such mechanism for designating directory (although I wish there was). Alternatively all consumers can configure through .flowconfig to use alternate directory per package bases but that get's really messy and breaks with transitive dependencies.

[mikeal]

It checks only files that start with // @flow however if package does not contain .flowconfig file it will error.

Sorry, what i meant was, if we have a flow type file and someone ads a new function to the .js file and there are no type defs, will it error or just ignore the new function?

[Gozala]

From flow perspective there is no correlation between two - so it won’t error.

Which is also a problem with this approach because things can get out of sync just like they do with docs

[mikeal]

Which is also a problem with this approach because things can get out of sync just like they do with docs

Ya, we had the same problem with jsdoc and that’s why I’m pushing us not to use it anymore. But I’m willing to try anything out if someone gets value out of it and it doesn’t have any impact on other users and developers. If it ends up being out of date in a year from now we can always consider removing it.

[Gozala]

Updated pull request to fix typos and fix Reader interface that I mistook for async.

[Gozala]

Ya, we had the same problem with jsdoc and that’s why I’m pushing us not to use it anymore. But I’m willing to try anything out if someone gets value out of it and it doesn’t have any impact on other users and developers. If it ends up being out of date in a year from now we can always consider removing it.

That's why I think inline comments give you the best deal:

1. Consumers don't have to care
2. If consumers type-check then they get a superior experience.
3. Code is always in sync
4. You can ensure APIs conform to interface and miss-matches are reported in pull requests.
5. Code runs as is in JS engines.

[mikeal]

Code is always in sync

Our experience with jsdoc has been that, even when the type defs are next to the code, they are not assured to stay in line with the implementation ☹️

[Gozala]

Code is always in sync

Our experience with jsdoc has been that, even when the type defs are next to the code, they are not assured to stay in line with the implementation ☹️

Flow type-checker will run on every change and will fail pull if things are out of sync, so I guarantee experience will be different here.

Clarification: It will be different assuming inline type comments are used and type checker runs along with lint.

[mikeal]

I only agreed to add the type defs with the understanding that new contributors wouldn’t have to worry about them. If we add type checking to the default test phase then they will need to modify them.

flow is not used widely enough to expect new contributors to already know it and adding it as a requirement is a significant barrier to entry for new contributors.

[Gozala]

I only agreed to add the type defs with the understanding that new contributors wouldn’t have to worry about them. If we add type checking to the default test phase then they will need to modify them.

Only if they make breaking API change, types are only needed for exported interface, everything else is inferred.

flow is not used widely enough to expect new contributors to already know it and adding it as a requirement is a significant barrier to entry for new contributors.

It’s mostly compatible with TS (except of few exceptions) which I’d argue is increasingly popular & widely used.

I do however agree with general sentiment that is - significant changes would require understanding types, I don’t agree bar is that high however, nor that most contributions will have to know or care about type-checker. And if some contributors see errors, it’s arguably better to work with contributor to resolve issues than not catch it in first place.

I honestly find all the various commit / markdown / formatting lints used a lot more difficult to deal with than with flow

[Gozala]

I honestly find all the various commit / markdown / formatting lints used a lot more difficult to deal with than with flow

That came out as a rant, I apologize. What I meant to say is that flow generally produces pretty helpful messages explaining what’s wrong, much more so than many linters used across PL projects in my experience.

There is also many ways to reduce bar by say integrating prettier in place of standard (I personally find spending more time trying to satisfy formatting than writing code & unlike actual issues it’s mostly spaceing)

Obviously type miss-matches could be treated as non-blockers & pulls can be accepted either way, However making everyone aware of the errors is helpful even if contributor isn’t necessarily required to address them.

[mikeal]

Only if they make breaking API change, types are only needed for exported interface, everything else is inferred.

Ah, I see, thanks for clarifying. Maybe we should have just waited a week or so until the API stabilized a bit before adding the definition. Oh well, I think we’re done with most major API changes, most of the stuff currently under consideration are API additions.

[Gozala]

Hey @mikeal what do you want to do with this pull request ?

[mikeal]

I don’t have a strong opinion, but I’m a little worried about this falling out of line with the implementation since we’re still iterating a lot and it sounds like you may not have the bandwidth to continue working on it in the short term. I think there are even new APIs added since the last update to this PR. All of these files will eventually move to other repos as well, so I’m not sure what the utility is in having this in this short term period where churn is high.

@vmx what do you think?

[Gozala]

My hope was that this would incentivize to encode API in interface definitions instead of ambiguous markdown defs. If there is no interest in keeping interface definitions be an actual API definition then probably it does not make much sense to keep it around.

[vmx]

I agree with @Gozala here, that types help with designing the APIs. If it's hard to express with types, it's probably hard to reason about, it's probably not the best API.