Copy flow files so that the module returns typed values #51

rickharris · 2018-12-18T15:55:52Z

We've had a bit of conversation about how we should enable external flow type checking when using this module, and although we're not in agreement quite yet, I am opening this PR for discussion of its merits and because it unblocks me in trying to incorporate this module into our projects.

The problem

By using index.js.flow as we would normally use a types.js file for all of our types, we end up exporting our types, but they're not actually used when we import non-types. For instance, if you import

import * as Abstract from "abstract-sdk";

Abstract is any.

The possible solution in this PR

Move the current index.js.flow to types.js and use flow-copy-source to copy uncompiled source code to lib, suffixed with .flow. This way, when you import from abstract-sdk, the actual code comes from .js files, but flow knows to look in their corresponding .js.flow files for typings. This is a really simple way to tell flow the types of the things that come from your module.

Now we have type checking and even completions working

Tracking some issues using the module with flow enabled

By returning AbstractInterface from Abstract.Client, you can't actually call something like client.projects.list, because projects is a maybe on AbstractInterface.

bitpshr · 2018-12-18T16:33:15Z

Thanks for putting this together, it's good to see how this approach looks in practice. It's very surprising to me that the Flow compiler doesn't have any option to generate public types for each module it encounters. In TypeScript, .d.ts files are placed alongside compiled .js files by default, making both type generation and consumption mostly automatic (similar to something like source maps.) I noticed early versions of Flow included the seemingly-broken gen-flow-files command, but it doesn't appear to be explicitly supported or documented anymore.

One issue with the flow-copy-source approach is resulting module size. By copying uncompiled source code instead of generating minimal type definitions, we're increasing the resulting package size in a non-negligible manner to appease downstream compilers.

Thinking out loud: how do other major production projects generate typings for their compiled code using Flow? This seems like an incredibly important feature for Flow to omit. Imagine a massive project with hundreds of files that will ultimately be distributed via NPM; are the two options really to copy entire source trees or to hand-roll types?

rickharris · 2018-12-18T16:37:47Z

We might take a look at how React does it, as they're probably the biggest project that does Flow typing. Although since they're Facebook, too, they might even have React types built into flow at this point :\

bitpshr · 2018-12-18T17:17:13Z

What about using Babel to convert type annotations to comments? This may be a happy medium: we only ship built code, but the built code contains commented-out type annotations that are still picked up by Flow. I'd personally be OK with uglier (and larger) compiled code if it included type annotations, especially since the type comments could be stripped out in downstream projects as long as the projects are built (regardless of whether they use Flow.)

rickharris · 2018-12-18T17:29:50Z

I'm game, didn't even know that babel plugin existed.

Looks like react types are indeed built directly into flow.

rickharris · 2018-12-18T17:47:11Z

So I gave @babel/plugin-transform-flow-comments a shot and... it doesn't work.

First thing I noticed - the compiled files still don't have a @flow pragma, so Flow doesn't even know to type check them. I have no idea why that plugin wouldn't add the flow pragma, but that's not the worst of it.

Secondly, it seems like flow comments either don't work with all flow code, or either the plugin itself doesn't cover all of flow's language feature. After compiling abstract-sdk with this plugin enabled, it spit out code with syntax errors:

ERROR in ./node_modules/@elasticprojects/abstract-core/node_modules/abstract-sdk/lib/AbstractAPI/index.js
Module parse failed: Unexpected token (68:18)
You may need an appropriate loader to handle this file type.
| }
|
| class AbstractAPI implements {
|   constructor({
|     /*: Options*/

The implements keyword is a flow thing and isn't getting handled properly here.

amccloud · 2018-12-18T18:40:04Z

Not sure I see the need to move away from index.js.flow

Could we add a libdef to it and make sure it gets bundled in the package?

Most and Ava do something similar:

rickharris · 2018-12-18T19:23:49Z

@amccloud 100% down to try the libdef approach.

One thing I want to point out, though, is that neither of those projects use flow internally. They're just providing a libdef. The copy source method is useful when you use flow internally and want other people to be able to use your types, basically without you having to exert any extra effort, since your exported API is already typed.

rickharris · 2018-12-18T22:46:11Z

I'd like to make one last argument in favor of this PR & flow-copy-source: the libdef is going to be harder to write, and this method is so easy and it just works. The only downfall of moving index.js.flow to types.js and using flow-copy-source is that it does double the amount of code in the published npm module, but two things to keep in mind:

It doesn't change the compiled size of code that uses it
For now, at the very least, this really isn't a problem because this project is not that big.

cc @amccloud @bitpshr

amccloud · 2018-12-19T01:10:05Z

@rickharris seems worth it if you get a libdef for free! Can we ever opt-out of publishing a type or is any type we export internally also a public type because it will be copied?

rickharris · 2018-12-19T06:03:08Z

Can we ever opt-out of publishing a type or is any type we export internally also a public type because it will be copied?

That's a good question. We could control what gets exported from the index. So say that we have src/types.js and it has a type called AbstractInternalThing and we don't really want to publish it, you couldn't import it like import type { AbstractInternalThing } from 'abstract-sdk'. But you could import it as import type { AbstractInternalThing } from 'abstract-sdk/lib/types', if someone read the source code and decided they wanted to do that. Usually that kind of thing (things inside of lib directories) in modules is considered private/internal. If we specifically did want to export a certain type, we could export type { AbstractExternalThing } from './types' and that would do the trick.

It might also be possible to only copy index.js using flow-copy-source such that only the public-facing API was available for being typed, but I'm not sure if that would 100% work or not. Like say inside of index.js we have export { Client } from './Client', then I'm not sure Client would be typed unless we also copied Client/index.js as Client/index.js.flow. We could test that out.

rickharris · 2018-12-20T17:07:07Z

@amccloud do you have thoughts on the issue I described in the main PR description?

By returning AbstractInterface from Abstract.Client, you can't actually call something like client.projects.list, because projects is a maybe on AbstractInterface

amccloud · 2018-12-21T00:01:27Z

@rickharris that was sorta intentional because depending on the transport the endpoint may not be available. We could make AbstractCLI implement everything but stub/throw if something not supported by cli is called?

rickharris · 2018-12-21T04:56:08Z

🤔 seems less than ideal for the default AUTO case which will have everything. Assuming that AbstractInterface was mostly meant to be the interface that the transports implement, we could probably return the actual type of whatever transport is returned from Abstract.Client so that a particular function can be either defined or undefined on the transport, but not a maybe.

rickharris · 2019-01-08T22:18:47Z

Had a convo with @amccloud and there's still a lot to figure out about our exported types for this package, but in the meantime we might as well merge this so we have types for the existing code.

tommoor

Tested locally, it works. Seems like a reasonable solution

✅ from me

amccloud · 2019-01-08T23:13:53Z

package.json

@@ -12,7 +12,7 @@
  },
  "scripts": {
    "prepublish": "yarn run build",
-    "build": "babel ./src -d ./lib",
+    "build": "babel ./src -d ./lib && flow-copy-source src lib",


would be nice to switch the order of these so the babel options like --watch can still be used with build

also nit: ./src ./lib to match all the other relative paths

Fair point. We'd probably want a kind of watch command to do both of these things, wouldn't we?

yea something like that or like start for development

amccloud · 2019-01-08T23:24:37Z

src/types.js

-    info: (
-      userDescriptor: UserDescriptor
-    ) => Promise<User>
+    info: (userDescriptor: UserDescriptor) => Promise<User>
  };
 }


Should we export some/all of these types in index.js so that we can have access to Abstract.ShareDescriptor or Abstract.types.ShareDescriptor in application code

amccloud

Works for me!

Just one comment about exporting some of the types for applications

bitpshr

This works nicely and feels like a good immediate solution.

rickharris · 2019-01-08T23:55:20Z

@amccloud we could export whatever types we need, sure, but I'm not 100% convinced by that example. I'd rather see that component define it's own requirements and hide its internal usage of abstract-sdk. For instance, if it ever needed another ID passed to it that wasn't part of ShareDescriptor, it'd be harder & more awkward to use that type.

amccloud · 2019-01-09T00:38:15Z

@rickharris descriptors are public API https://sdk.goabstract.com/docs/abstract-api/#descriptors

Individual properties like projectId, branchId, sha, etc. are now internals, so components should no longer depend on those

Edit:

You could still depend on individual properties if you need to because their type is just a string

rickharris · 2019-01-09T00:52:06Z

@amccloud I'm confused about what points you're making here. Descriptors are still just objects of string ids, like you said in your edit, right?

amccloud · 2019-01-09T00:58:14Z

@rickharris descriptors are now valid dependencies too, so components should prefer to depend on descriptors if they can:

type Props = {
 params: Abstract.LayerDescriptor,
 layer: Abstract.Layer // would be nice too
};

amccloud · 2019-01-09T01:05:56Z

src/types.js

@@ -55,7 +55,7 @@ export type PageDescriptor = {|

 export type LayerDescriptor = {|
  ...ObjectDescriptor,
-  sha: $PropertyType<ObjectDescriptor, "sha"> | "latest",
+  sha: $PropertyType<ObjectDescriptor, "sha">,


Adding a note here for future us. Flow doesn't appear to be refining the literal "latest" when a desscriptor (like CommitDescriptor) and descriptor that supports latest are in a union

amccloud · 2019-01-09T19:20:34Z

@rickharris I really think we should export all of the types that are documented and intended to be public API. It would be the types in the right sidebar here https://sdk.goabstract.com/docs/abstract-api/ and the subtypes we document like ChangesetChange, CollectionLayer, Annotation, etc.

This PR could benefit from the types https://github.com/goabstract/ui/pull/202 and it was the intention all along with index.js.flow

bitpshr · 2019-01-09T20:33:34Z

I agree with @amccloud. From a high level, consumers of a typed API should be able to use the same interfaces that make up that public API.

rickharris · 2019-01-10T22:41:08Z

@amccloud check out cfc83bb and 2760ed1. I think this should be good to go now.

Copy flow files so that the module returns typed values

5d9b61e

Merge branch 'master' into flow-copy-source

8c260c1

rickharris force-pushed the flow-copy-source branch from 8ec4257 to 8c260c1 Compare December 20, 2018 16:52

rickharris added 4 commits December 20, 2018 11:59

Try to fix the yarn lock file

a438c56

Merge branch 'master' into flow-copy-source

ca4ec40

👕 lint fix

d4b9340

Fixing bad merge

b9360fa

Merge branch 'master' into flow-copy-source

d6c9d0d

rickharris requested review from amccloud, tommoor and bitpshr January 8, 2019 22:18

tommoor approved these changes Jan 8, 2019

View reviewed changes

amccloud reviewed Jan 8, 2019

View reviewed changes

amccloud approved these changes Jan 8, 2019

View reviewed changes

bitpshr approved these changes Jan 8, 2019

View reviewed changes

Remove 'latest' string literal from descriptor types

cb8cb1e

amccloud reviewed Jan 9, 2019

View reviewed changes

rickharris added 3 commits January 10, 2019 14:09

Merge branch 'master' into flow-copy-source

82c1933

Move flow-copy-source to prebuild and ignore test files

2760ed1

Export types from module index

cfc83bb

rickharris merged commit 23b6043 into master Jan 16, 2019

rickharris deleted the flow-copy-source branch January 16, 2019 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy flow files so that the module returns typed values #51

Copy flow files so that the module returns typed values #51

rickharris commented Dec 18, 2018 •

edited

Loading

bitpshr commented Dec 18, 2018 •

edited

Loading

rickharris commented Dec 18, 2018

bitpshr commented Dec 18, 2018 •

edited

Loading

rickharris commented Dec 18, 2018

rickharris commented Dec 18, 2018

amccloud commented Dec 18, 2018

rickharris commented Dec 18, 2018

rickharris commented Dec 18, 2018 •

edited

Loading

amccloud commented Dec 19, 2018

rickharris commented Dec 19, 2018

rickharris commented Dec 20, 2018

amccloud commented Dec 21, 2018

rickharris commented Dec 21, 2018

rickharris commented Jan 8, 2019

tommoor left a comment

amccloud Jan 8, 2019

amccloud Jan 8, 2019

rickharris Jan 8, 2019

amccloud Jan 8, 2019

amccloud Jan 8, 2019 •

edited

Loading

amccloud left a comment

bitpshr left a comment •

edited

Loading

rickharris commented Jan 8, 2019

amccloud commented Jan 9, 2019 •

edited

Loading

rickharris commented Jan 9, 2019

amccloud commented Jan 9, 2019 •

edited

Loading

amccloud Jan 9, 2019

amccloud commented Jan 9, 2019 •

edited

Loading

bitpshr commented Jan 9, 2019

rickharris commented Jan 10, 2019

Copy flow files so that the module returns typed values #51

Copy flow files so that the module returns typed values #51

Conversation

rickharris commented Dec 18, 2018 • edited Loading

The problem

The possible solution in this PR

Tracking some issues using the module with flow enabled

bitpshr commented Dec 18, 2018 • edited Loading

rickharris commented Dec 18, 2018

bitpshr commented Dec 18, 2018 • edited Loading

rickharris commented Dec 18, 2018

rickharris commented Dec 18, 2018

amccloud commented Dec 18, 2018

rickharris commented Dec 18, 2018

rickharris commented Dec 18, 2018 • edited Loading

amccloud commented Dec 19, 2018

rickharris commented Dec 19, 2018

rickharris commented Dec 20, 2018

amccloud commented Dec 21, 2018

rickharris commented Dec 21, 2018

rickharris commented Jan 8, 2019

tommoor left a comment

Choose a reason for hiding this comment

amccloud Jan 8, 2019

Choose a reason for hiding this comment

amccloud Jan 8, 2019

Choose a reason for hiding this comment

rickharris Jan 8, 2019

Choose a reason for hiding this comment

amccloud Jan 8, 2019

Choose a reason for hiding this comment

amccloud Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

amccloud left a comment

Choose a reason for hiding this comment

bitpshr left a comment • edited Loading

Choose a reason for hiding this comment

rickharris commented Jan 8, 2019

amccloud commented Jan 9, 2019 • edited Loading

rickharris commented Jan 9, 2019

amccloud commented Jan 9, 2019 • edited Loading

amccloud Jan 9, 2019

Choose a reason for hiding this comment

amccloud commented Jan 9, 2019 • edited Loading

bitpshr commented Jan 9, 2019

rickharris commented Jan 10, 2019

rickharris commented Dec 18, 2018 •

edited

Loading

bitpshr commented Dec 18, 2018 •

edited

Loading

bitpshr commented Dec 18, 2018 •

edited

Loading

rickharris commented Dec 18, 2018 •

edited

Loading

amccloud Jan 8, 2019 •

edited

Loading

bitpshr left a comment •

edited

Loading

amccloud commented Jan 9, 2019 •

edited

Loading

amccloud commented Jan 9, 2019 •

edited

Loading

amccloud commented Jan 9, 2019 •

edited

Loading