`flow shadow-file` #2184

jeffmo · 2016-08-03T17:18:06Z

This adds a new command, flow shadow-file path/to/file.js, which generates the minimal .js.flow interface file that can be used for publishing compiled Flow source. Specifically: It generates only declare and declare export statements in order to omit all the implementation text.

Background: Currently the best practice for publishing Flow source is (roughly):

cp src/foo.js dist/foo.js.flow
babel src/foo.js > dist/foo.js

This mostly works, but has a few downsides:

The published contents of dist/* are much larger than they need to be -- they contain a pre-compiled copy of all the original source. This is basically just crufty bytes that need to be downloaded every time the package is npm install-ed.
The published .js.flow files contain implementation details -- which are much more susceptible to breaking changes across Flow versions. While breaking changes across Flow version are still possible even in declare statements, this happens far less often.
We also want to re-use this type -> code codegen infrastructure to generate libdefs for flow-typed at some point. This particular diff is only about .js.flow files (i.e. no declare module ... statements) -- but that can be added on in a follow up diff.

This diff includes a new codegen.ml file which exposes an API intended for generating code from various things (types for now, possibly ASTs at some point if that's useful/performant/worth the effort). It's minimal in terms of feature-set right now but we can expand/improve it later. I didn't use type_printer.ml because it only handles some types and many of the types it will print aren't actual code or easily composable with other bits of code. It's also used by lots of stuff that I didn't really want to investigate breaking changes for while building out this feature.

At some point it probably makes sense to either improve Type_printer enough to subsume Codegen or the other way around. I suspect the Codegen is going to be easier to generalize, but we'll leave that for a later time to look into.

jeffmo · 2016-08-03T17:18:13Z

@facebook-github-bot import

ghost · 2016-08-03T17:19:04Z

Thanks for importing.

jeffmo · 2016-08-03T17:23:56Z

@facebook-github-bot update

mroch · 2016-08-03T17:34:37Z

might I suggest the name flow declare or flow gen-decl or flow gen-def or flow interface or something like that? I think it should a) be a verb if possible, and b) not introduce new terminology that isn't used elsewhere ("shadow file").

mroch · 2016-08-03T17:38:37Z

src/commands/shadowFileCommand.ml

@@ -0,0 +1,66 @@
+open CommandUtils


missing copyright header

jeffmo · 2016-08-05T17:57:44Z

might I suggest the name flow declare or flow gen-decl or flow gen-def or flow interface or something like that? I think it should a) be a verb if possible, and b) not introduce new terminology that isn't used elsewhere ("shadow file").

So I'd like us to get away from using declare and def/definition to describe .js.flow files. I would like to update this in the docs as well (and I can & should do that in this diff if we can agree on shadow or something else).

Specifically: There's a lot of confusion right now around the differences between .js.flow files (things that shadow .js files) and "libdefs" (things that go into the [libdefs] section of your flowconfig).

I wrote up some sample documentation here that I'd like to move into docs soon (maybe in this diff): #1996 (comment)

I'm not necessarily tied to shadow, but it makes the most sense to me: They're files that shadow adjacent/same-named .js files.

jeffmo · 2016-08-05T17:59:36Z

I should be able to circle back and update the "libdef" and "shadow files" docs either this weekend or Monday.

jeffmo · 2016-08-06T17:25:42Z

copyright headers added
cprintf comment updated

jeffmo · 2016-08-06T18:03:35Z

Switched cprintf functions to take out_channel instead of a bool flag
Renamed files

mroch · 2016-08-06T18:23:11Z

Thanks for the updates.

I'm still not convinced on the name "shadow-file". You've previously suggested that users should write .js.flow files and compile them to .js files. The term "shadow" makes it feel like a second-class thing when it's really the other way around. Also, my understanding is that this command generates declare interfaces, not a complete shadow file as you've defined that term in the docs you linked to. flow-copy-source does what I think flow shadow-file should do based on its name.

jeffmo · 2016-08-06T18:54:23Z

The term "shadow" makes it feel like a second-class thing when it's really the other way around.

I've sort of gone back and forth on this, too. At the moment I'm stuck on the "it's not perfect, but not that unclear" side (you write a flow file that shadows the real, compiled, JS file).

my understanding is that this command generates declare interfaces

That's right. Depending on how you interpret "shadow", this still seems to make sense to me -- but maybe I've just gotten used to it after thinking about it for a little while now.

I'm still pretty open to names other than "shadow" if you have any suggestions, though? "shadow" is just the best I was able to come up with so far.

The only things I want to avoid are the terms "lib"/"def" (too confusingly similar to "[libdefs]"), and "decl"/"declare" (doesn't adequately distinguish libdefs, which have declares, from these files, which have declares).

I suppose we could name the command flow gen-flow-file?

jeffmo · 2016-08-06T18:58:24Z

cc @marudor @ryyppy for some outside perspectives: Any thoughts on what we should call ".js.flow" files?

(the files that get published to npm and sit next to .js files to "shadow" them -- for lack of a better term at the moment :) )

jeffmo · 2016-08-06T19:04:24Z

src/typing/codegen.ml

+  declared_classes: string IMap.t;
+  mutable next_declared_class_name: int;
+  flow_cx: Context.t;
+  indent: string;


unused right now

ryyppy · 2016-08-06T19:09:33Z

When I initially saw the PR with shadow-file, I knew what this is about before I even saw the PR details (although I might be biased).

Usually when I explain libdefs / js.flow files to others, I usually called them header files (like in C) on either lib or file level... but I guess this is even more confusing...

mroch · 2016-08-06T19:17:37Z

I don't think we're discussing what to call all kinds of .js.flow files here, only what to call .js.flow files which happen to contain only declare statements.

I think we should call these declaration files, because they only contain declarations. "libdefs" from the builtins or [libs] are just global declaration files, and we could start calling them such (and should rename them to .js.flow).

I'd call all forms of .js.flow files something like "Flow files."

samwgoldman · 2016-08-06T19:24:59Z

This is great. I am especially happy that this encoding will prevent issues where config differences between lib and consumer cause errors, as you pointed out in your description.

Does this work for files that import types/values from other modules? Can you include some tests for generating shadow files for modules with dependencies?

Since you're generating the declarations from types, I assume suppression types just come out as any, correct? Type aliases/annots are substituted by their definition as well, yes?

What about generating shadow files for many modules at once? I think that could be a common use case, for example fbjs's build step copies over all files. Is there any perf gain to be had with a batch mode?

samwgoldman · 2016-08-06T19:31:35Z

src/typing/codegen.ml

+  let id = env.next_declared_class_name in
+  env.next_declared_class_name <- id + 1;
+  id
+let resolve_type t env = Flow_js.resolve_type env.flow_cx t


On mobile so forgive an unresearched comment: I think Flow_js.resolve_type will return any for open tvars, so ideally this should happen after merge. Does it?

Right, the expectation is that the cx passed into Codegen.mk_env is fully merged.

jeffmo · 2016-08-06T19:39:35Z

only what to call .js.flow files which happen to contain only declare statements.

This is a level of nuance that we want to avoid. The mental model needs to be simple, and currently it is: .js.flow files always shadow .js files and can be anything a .js file can be. Libdefs are global and registered in the config.

People seem to understand this model once described in this way, so I want to avoid breaking things down into any further categories of files. TS's story around interface/decl/typings files is all over the place with terminology and I've always found it confusing in conversations about them -- so this has motivated a strong need to keep things simple and minimal as we build out our story here.

jeffmo · 2016-08-06T19:47:54Z

Can you include some tests for generating shadow files for modules with dependencies?

Good call, I'll add some tests. In the beginning I expect these imported types to simply be flattened into the declared types, but once we work out our story around imports between libdefs we can take that approach.

Since you're generating the declarations from types, I assume suppression types just come out as any, correct? Type aliases/annots are substituted by their definition as well, yes?

That's right, but I should add tests to prove it

What about generating shadow files for many modules at once?

Right now this only takes a single file on the command line and prints the results to stdout (first-version simplicity). The server command is intentionally set up to accept multiple files and return multiple results to support multi-file generation though. It might be a little better to do this in a follow-up.

samwgoldman · 2016-08-06T19:54:20Z

These files can include imports though, right? They're JS files including declarations, not lib defs iiuc. In fact, you probably need that in order to use class types (nominal) from other modules.

jeffmo · 2016-08-06T19:57:30Z

Oh, good point -- they can include imports, you're right. I'll need to work that in then

mroch · 2016-08-07T00:16:55Z

This is the level of nuance we want to avoid. The mental model needs to be simple, and currently it is: flow files always shadow js files and can be anything a js file can be.

So I agree with that mental model, but this command doesn't generate all types of shadow files, it generates a very specific subset: the declarations/interface of some real JS file.

Why does a command called shadow-file or gen-flow-file only generate interfaces? From the name and your description of shadow files, why doesn't it just copy the .js file to .js.flow (maybe even adding missing types, subsuming the suggest command)? That would also be a valid shadow file.

All I'm suggesting is that the name should reflect what the command does.

jeffmo · 2016-08-07T01:27:14Z

Why does a command called shadow-file or gen-flow-file only generate interfaces?

The purpose of the command is to generate a .js.flow file that is semantically identical to the .js file -- so whether this command does cp or whether it generates declares is basically an implementation detail in terms of that goal. The command chooses declares because they offer the best bang for the buck in that goal (version compat, minimal bytes, etc).

Truthfully though, I don't think it's necessary to be completely precise in the name as long as it's not incorrect. In this case, generating a shadow/.js.flow file is what we're doing here -- it's just left as an implicit detail (easily notable in the --help text and docs) that we're generating the minimal form. I can't think of any good reason for this command to be a new version of cp, though.

If it's any consolation: If we ever do discover a good reason to generate non-declare-only .js.flow files, we can always add a flag that instructs the command to codegen alternative forms (i.e. --fullImpl, etc).

Summary: This adds a new command, `flow gen-flow-files path/to/file.js`, which generates the minimal `.js.flow` interface file that can be used for publishing compiled Flow source. Specifically: It generates only `import`, `declare`, and `declare export` statements in order to omit all the implementation text. Background: Currently the best practice for publishing Flow source is (roughly): 1. `cp src/foo.js dist/foo.js.flow` 2. `babel src/foo.js > dist/foo.js` This mostly works, but has a few downsides: 1. The published contents of `dist/*` are much larger than they need to be -- they contain a pre-compiled copy of all the original source. This is basically just crufty bytes that need to be downloaded every time the package is `npm install`-ed. 2. The published `.js.flow` files contain implementation details -- which are much more susceptible to breaking changes across Flow versions. While breaking changes across Flow version are still possible even in `declare` statements, this happens *far* less often. 3. We also want to re-use this type -> code codegen infrastructure to generate libdefs for flow-typed at some point. This particular diff is only about `.js.flow` files (i.e. no `declare module ...` statements) -- but that can be added on in a follow up diff. This diff includes a new `codegen.ml` file which exposes an API intended for generating code from various things (types for now, possibly ASTs at some point if that's useful/performant/worth the effort). It's minimal in terms of feature-set right now but we can expand/improve it later. I didn't use `type_printer.ml` because it only handles some types and many of the types it will print aren't actual code or easily composable with other bits of code. It's also used by lots of stuff that I didn't really want to investigate breaking changes for while building out this feature. At some point it probably makes sense to either improve `Type_printer` enough to subsume `Codegen` or the other way around. I suspect the `Codegen` is going to be easier to generalize, but we'll leave that for a later time to look into. Closes facebook/flow#2184 Reviewed By: gabelevi Differential Revision: D3663107 Pulled By: jeffmo fbshipit-source-id: a791f85235f978fc9e5e46639e0dec37b71fad60

Summary: This adds a new command, `flow gen-flow-files path/to/file.js`, which generates the minimal `.js.flow` interface file that can be used for publishing compiled Flow source. Specifically: It generates only `import`, `declare`, and `declare export` statements in order to omit all the implementation text. Background: Currently the best practice for publishing Flow source is (roughly): 1. `cp src/foo.js dist/foo.js.flow` 2. `babel src/foo.js > dist/foo.js` This mostly works, but has a few downsides: 1. The published contents of `dist/*` are much larger than they need to be -- they contain a pre-compiled copy of all the original source. This is basically just crufty bytes that need to be downloaded every time the package is `npm install`-ed. 2. The published `.js.flow` files contain implementation details -- which are much more susceptible to breaking changes across Flow versions. While breaking changes across Flow version are still possible even in `declare` statements, this happens *far* less often. 3. We also want to re-use this type -> code codegen infrastructure to generate libdefs for flow-typed at some point. This particular diff is only about `.js.flow` files (i.e. no `declare module ...` statements) -- but that can be added on in a follow up diff. This diff includes a new `codegen.ml` file which exposes an API intended for generating code from various things (types for now, possibly ASTs at some point if that's useful/performant/worth the effort). It's minimal in terms of feature-set right now but we can expand/improve it later. I didn't use `type_printer.ml` because it only handles some types and many of the types it will print aren't actual code or easily composable with other bits of code. It's also used by lots of stuff that I didn't really want to investigate breaking changes for while building out this feature. At some point it probably makes sense to either improve `Type_printer` enough to subsume `Codegen` or the other way around. I suspect the `Codegen` is going to be easier to generalize, but we'll leave that for a later time to look into. Closes #2184 Reviewed By: gabelevi Differential Revision: D3663107 Pulled By: jeffmo fbshipit-source-id: 8f1147590e8bf48ebedb7b6ea5d1b720669c7518

ghost added the CLA Signed label Aug 3, 2016

jeffmo force-pushed the gen_interface branch from a9a2ab2 to f285c07 Compare August 3, 2016 17:19

mroch reviewed Aug 3, 2016
View reviewed changes

src/commands/shadowFileCommand.ml

@@ -0,0 +1,66 @@

open CommandUtils

Copy link

Contributor

mroch Aug 3, 2016

Choose a reason for hiding this comment

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

missing copyright header

ryyppy mentioned this pull request Aug 5, 2016

Additional Flow Checking? salsita/prism#20

Open

jeffmo force-pushed the gen_interface branch 2 times, most recently from 12a5932 to 78678f2 Compare August 6, 2016 17:24

jeffmo force-pushed the gen_interface branch 2 times, most recently from 7788a38 to e6dad12 Compare August 6, 2016 18:02

flow shadow-file

a863e61

jeffmo force-pushed the gen_interface branch from e6dad12 to a863e61 Compare August 6, 2016 18:16

jeffmo reviewed Aug 6, 2016
View reviewed changes

samwgoldman reviewed Aug 6, 2016
View reviewed changes

jamiebuilds mentioned this pull request Aug 8, 2016

Flow types in RelayTypes stripped from npm module facebook/relay#1064

Closed

ghost closed this in 4748862 Sep 1, 2016

gabro mentioned this pull request Jan 5, 2017

#9: [CustomerAPI] refactor using ES6 classes (closes #9) contactlab/contacthub-sdk-nodejs#15

Merged

goodmind added the Merged label Jul 29, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`flow shadow-file` #2184

`flow shadow-file` #2184

jeffmo commented Aug 3, 2016

jeffmo commented Aug 3, 2016

ghost commented Aug 3, 2016

jeffmo commented Aug 3, 2016

mroch commented Aug 3, 2016

mroch Aug 3, 2016

jeffmo commented Aug 5, 2016

jeffmo commented Aug 5, 2016

jeffmo commented Aug 6, 2016

jeffmo commented Aug 6, 2016 •

edited

Loading

mroch commented Aug 6, 2016

jeffmo commented Aug 6, 2016

jeffmo commented Aug 6, 2016

jeffmo Aug 6, 2016

ryyppy commented Aug 6, 2016

mroch commented Aug 6, 2016

samwgoldman commented Aug 6, 2016

samwgoldman Aug 6, 2016

jeffmo Aug 6, 2016

jeffmo commented Aug 6, 2016 •

edited

Loading

jeffmo commented Aug 6, 2016

samwgoldman commented Aug 6, 2016

jeffmo commented Aug 6, 2016 •

edited

Loading

mroch commented Aug 7, 2016

jeffmo commented Aug 7, 2016 •

edited

Loading

flow shadow-file #2184

flow shadow-file #2184

Conversation

jeffmo commented Aug 3, 2016

jeffmo commented Aug 3, 2016

ghost commented Aug 3, 2016

jeffmo commented Aug 3, 2016

mroch commented Aug 3, 2016

mroch Aug 3, 2016

Choose a reason for hiding this comment

jeffmo commented Aug 5, 2016

jeffmo commented Aug 5, 2016

jeffmo commented Aug 6, 2016

jeffmo commented Aug 6, 2016 • edited Loading

mroch commented Aug 6, 2016

jeffmo commented Aug 6, 2016

jeffmo commented Aug 6, 2016

jeffmo Aug 6, 2016

Choose a reason for hiding this comment

ryyppy commented Aug 6, 2016

mroch commented Aug 6, 2016

samwgoldman commented Aug 6, 2016

samwgoldman Aug 6, 2016

Choose a reason for hiding this comment

jeffmo Aug 6, 2016

Choose a reason for hiding this comment

jeffmo commented Aug 6, 2016 • edited Loading

jeffmo commented Aug 6, 2016

samwgoldman commented Aug 6, 2016

jeffmo commented Aug 6, 2016 • edited Loading

mroch commented Aug 7, 2016

jeffmo commented Aug 7, 2016 • edited Loading

`flow shadow-file` #2184

`flow shadow-file` #2184

jeffmo commented Aug 6, 2016 •

edited

Loading

jeffmo commented Aug 6, 2016 •

edited

Loading

jeffmo commented Aug 6, 2016 •

edited

Loading

jeffmo commented Aug 7, 2016 •

edited

Loading