Install defs from node_modules/<package>/flow-self-typed if it exists for each package #1435

jedwards1211 · 2017-10-31T19:30:03Z

This is a workaround for facebook/flow#4917. (Problem: It's impossible to provide different .js.flow files for different versions of flow in the same package.)

One can currently rely on PRing def updates for the various targeted flow versions to the central flow-typed repository instead of using .js.flow files, but this has downsides:

You have to wait for the PR to be merged
You can't do this for scoped private packages, at least not without revealing information about them

That's why I made this PR: this makes it possible to store definitions (which can target multiple versions of flow) in a flow-self-typed directory in the published package instead of making PRs to the central repo.

Notes

self-provided defs will only be found for packages that are already installed in node_modules!
if an installed package contains a flow-self-typed directory it will override anything in flow-typed's central repo.

Directory structure

To use this feature, include a flow-self-typed directory in the root of a published package.

Tue flow-self-typed directory must have the same structure as definitions/npm/<package>_v<version>/ in this repo would,
except that the .js files must use the exact version of the package:

flow-self-typed/
  flow_v0.13.x-v0.37.x/
    underscore_v1.8.3.js
  flow_v0.38.x-/
    underscore_v1.8.3.js
package.json
...

willmruzek · 2017-10-31T19:53:40Z

Wow! I hadn't thought of this. My only concern is that it might get confusing using flow-typed to both consume and publish types. How are the two cases differentiated?

jedwards1211 · 2017-10-31T19:56:31Z

@mruzekw well, we would need to update the wiki to document this way of publishing types for a library if this PR gets merged.

willmruzek · 2017-10-31T19:57:57Z

Yes, that sounds important. But I'm unclear on the differences now...could you clarify? The path and the directory tree in "Directory Structure" don't match.

jedwards1211 · 2017-10-31T20:01:08Z

@mruzekw I meant that path to be within this repo, not within the package in question. I tweaked the wording: "A package's flow-typed directory must have the same structure as definitions/npm/<package>_v<version>/ in this repo would"

willmruzek · 2017-10-31T20:04:42Z

So the published package tree needs to have the following?

node_modules/underscore/
  flow-typed/
    flow_v0.13.x-v0.37.x/
      underscore_v1.8.3.js
    flow_v0.38.x-/
      underscore_v1.8.3.js

jedwards1211 · 2017-10-31T20:05:19Z

ah no, the published package needs to have

  flow-typed/
    flow_v0.13.x-v0.37.x/
      underscore_v1.8.3.js
    flow_v0.38.x-/
      underscore_v1.8.3.js

(as a sibling of package.json)

Then when the package is installed there will be a node_modules/underscore/flow-typed directory.

willmruzek · 2017-10-31T20:06:34Z

Got it. Thanks for clarifying.

It's unlikely a project would check these into git and just generate it as part of the build?

jedwards1211 · 2017-10-31T20:07:42Z

@mruzekw yes, though if one needs to support multiple versions of flow, I don't think there's a very good way to autogenerate all the defs anyway...

jedwards1211 · 2017-10-31T20:08:37Z

@mruzekw oh I think I misunderstood, you mean you would prefer to generate the flow-typed directory in prepublish rather than checking it into git?

The only problem is I don't think there's an automated way to generate these types of lib defs. Copying .js files to .js.flow files works, but one can't merely copy .js files into the flow-typed directory because they need to contain declare module 'blah' statements...

willmruzek · 2017-10-31T20:12:06Z

I have something in the works with respect to generating flow types across multiple versions. It's not easy... In the proof of concept stage.

I mean you have to update all the flow types for every version with each change in package version anyway. Why not automate that?

jedwards1211 · 2017-10-31T20:13:39Z

Oh, that's cool. Yeah by all means if you have a way to automate it, go for it!

willmruzek · 2017-10-31T20:14:22Z

Cool, that seems like a separate choice from this PR anyway. Great work! Hope the maintainers and community will consider it.

willmruzek · 2017-10-31T20:17:09Z

Oh! One more question. Can I test my package(d) types with different versions of flow?

jedwards1211 · 2017-10-31T20:22:17Z

@mruzekw you mean using flow-typed run-tests? I believe that won't work with this since run-tests is designed to be run on the flow-typed repository itself. I'll have to think about the best way to support running tests on the packaged types.

gantoine · 2017-11-01T02:33:43Z

Oh wow, nice work! 💪 I'm gonna take some time to look over this change, and reach out to the others since this is a significant architectural change.

@marudor Thoughts on this change? 💭

jedwards1211 · 2017-11-01T03:42:09Z

@gantoine do you think we should allow the .js files in the package not to use the exact package version? That was just the way it came out when I reused some existing functions in flow-typed, but I figured it might be a good thing as it would force users to think about keeping their defs up-to-date.

TiddoLangerak · 2017-11-01T08:52:50Z

I'm just a passer-by, but I have my doubts about the folder structure: if you're a library publisher that wants to publish flow types, then chances are you're already using flow-typed for your own dependencies. Currently the flow-typed folder contains only 3rd party code, which you don't want to publish. This folder is likely in your .npmignore, .gitignore, or omitted from the files array in your package.json. Chances are also that this folder is treated differently by other tooling (e.g. excluded from linters).

If we now add our own type defs in there as well then we need to instruct our tools to treat half of the folder differently from the rest. Especially for include patterns this isn't very nice (such as the files array in package.json), since we now either need to enumerate all versions, or use glob patterns to match all the flow_* folders.

Now this is all not impossible to do, but I think it would be more ergonomically to have a single folder dedicated to own types. This could either be a new top-level folder, or a single folder within /flow-typed (e.g. /flow-typed/package/flow_v0.13.x-v0.37.x/underscore_v1.8.3.js).

jedwards1211 · 2017-11-01T13:34:35Z

That's a good point, I'm trying to think what other names would make sense for the own types folder...

jedwards1211 · 2017-11-01T14:53:48Z

@gantoine @marudor I'm thinking flow-self-typed might be a good name for a package's own types folder. What do you think?

jedwards1211 · 2017-11-02T21:07:55Z

@gantoine what do you think about calling the types folder flow-self-typed?

gantoine · 2017-11-03T00:22:17Z

In all honesty I'm not too sure about this solution.

At first glance I have to agree with the points Tiddo brought up, in that we can't hijack the flow-typed library for this use. As for flow-self-typed, I can't justify making such a change, or installing that as a best practice, without input from the core flow team. Unfortunately it's a busy time of year for everyone, so this will have a linger for a while.

At the very least I'll keep it open and we can keep discussing it, since it's an interesting idea to a real problem people are having. I'm sorry I can't be of more help, I just don't think I'm in a position to call the shots. 😞

jedwards1211 · 2017-11-03T00:34:01Z

I'm not sure if you fully understand the points that Tiddo brought up. Publishing I a package with its typedefs in a flow-typed folder is awkward for people's workflow. But I could change this PR to look for self-provided types in a package's flow-self-typed folder instead, and it wouldn't interfere with any existing functionality or be awkward to use. In other words what Tiddo mentioned doesn't mean this approach is doomed, just needs one little tweak. The only concern is the name for that folder, there are no other ramifications.

gantoine · 2017-11-03T00:40:43Z

Oh no I understand what the purpose of flow-self-typed is, I wasn't saying that his comments were why I was unsure of this change. From what I can tell your solution would work just fine in the wild.

My reservations are towards making such a change without input from the flow/flowtype team, or from library maintainers who have flow typedefs in their libraries. This would require changing the way people interact with flow and flow-typed, and I don't feel I'm in a position to make that call.

willmruzek · 2017-11-03T00:44:02Z

I'm just happy this issue is getting some attention. I wish the flow team were more proactive on this.

…

On Nov 2, 2017 7:40 PM, "Georges-Antoine Assi" ***@***.***> wrote: Oh no I understand what the purpose of flow-self-typed is, I wasn't saying that his comments were why I was unsure of this change. From what I can tell your solution would work just fine in the wild. My reservations are towards making such a change without input from the flow/flowtype team, or from library maintainers who have flow typedefs in their libraries. This would require changing the way people interact with flow and flow-typed, and I don't feel I'm in a position to make that call. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1435 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGn6kT3ArUDuM3lDriXKDpJU4CKUGgAks5symEPgaJpZM4QNTOY> .

jedwards1211 · 2017-11-03T00:45:32Z

To be perfectly clear, here is how it would work with what I'm proposing that avoids the problem Tiddo mentioned.

Let's say you're developing a package foo that depends on underscore. Here is what your repo would look like:

.npmignore
flow-self-typed/
  flow_v0.13.x-v0.37.x/
    foo_v1.0.0.js
  flow_v0.38.x-/
    foo_v1.0.0.js
flow-typed/
  npm/
    underscore_v1.x.x.js
lib/
  index.js
package.json

flow-typed controls the flow-typed/ directory and it still works for flow checking.
.npmignore would ignore the flow-typed directory, but flow-self-typed would get published with the package.

Then someone installs foo in their project, which also uses flow-typed/. At that point, their project looks like this:

node_modules/
  foo/
    flow-self-typed/
      flow_v0.13.x-v0.37.x/
        foo_v1.0.0.js
      flow_v0.38.x-/
        foo_v1.0.0.js
    lib/
      index.js
    package.json
  underscore/
    ...
package.json

Now they run flow-typed install. When looking for defs for foo, flow-typed sees that node_modules/foo/flow-self-typed is a directory; it then copies node_modules/foo/flow-self-typed/flow_v0.38.x-/foo_v1.0.0.js (assuming the user is using flow 0.38+) to the project's flow-typed/npm/foo_vx.x.x.js. For other packages, like underscore, it still copies the defs from the central repository.

Does that clarify things?

jedwards1211 · 2017-11-03T00:51:34Z

Also to be clear, we wouldn't have to make any changes to flow itself to support this. You do realize that?

gantoine · 2017-11-05T15:45:17Z

Yeah, there are no alternative plans for releasing defs for private packages are there? No plans to make versionable .js.flow files?

Not as far as I can tell. 😕

chrisgervang · 2017-11-06T19:08:37Z

I really like the proposed solution, and I'd prefer adding more flexibility into flow-typed over requiring private projects to fork and maintain. Wanting to use published types from private packages is not limited to companies, since it's only $7/m now for individuals to use that feature.

Regarding,

I don't feel like having the library maintainer version flow themselves is the best/should be the recommended approach

I feel the same, but can empathize with a package author who does want to maintain multiple versions of their types. To satisfy both, this could look for specific flow version directories first and then look for libdefs in the flow-self-typed directory.

That way, it's harder to get blocked. If package authors don't want to maintain more than one version of libdef, they don't have to. If someone needs to add backward compatibility, they can.

.npmignore
flow-self-typed/
  foo_v1.0.0.js
  flow_v0.13.x-v0.37.x/
    foo_v1.0.0.js
  flow_v0.38.x-/
    foo_v1.0.0.js
flow-typed/
  npm/
    underscore_v1.x.x.js
lib/
  index.js
package.json

chrisgervang · 2017-11-06T20:05:06Z

FWIW, I'm using the script written by @a-tokyo for now: facebook/flow#4323

jedwards1211 · 2017-11-07T15:26:33Z

@chrisgervang That's a good idea. I wish types from the flow-typed directory would override .js.flow types (facebook/flow#5233); if that were the case instead of having a flow-self-typed/foo_v1.0.0.js file, people could just continue relying on lib/index.js.flow for the flow-version-agnostic types, and it wouldn't interfere when people want to add flow-version-specific defs to flow-typed. (Right now the workaround is to make flow ignore the package in node_modules altogether)

jedwards1211 · 2017-11-29T05:40:56Z

@gantoine I didn't notice, looks like you guys have decided you'd like to adopt this approach in 3.0? If so I'll update the code to use the flow-self-typed directory.

Also I may need help designing/figuring out an approach for running tests in flow-self-typed.

jedwards1211 · 2017-11-29T05:44:39Z

Also @gantoine make sure no one mistakenly thinks this is ready and merges it -- it still uses the flow-typed directory inside a package, not the flow-self-typed directory.

…ists for each package This is a workaround for facebook/flow#4917. **Note**: self-provided defs will only be found for packages that are already installed in `node_modules`! It also enables package authors to update their type defs without having to make PRs to `flow-typed`. If an installed package provides its own `flow-typed` directory it will override anything in `flow-typed`'s central repo. ### Directory structure A package's `flow-typed` directory has the same structure as `flow-typed/definitions/npm/<package>_v<version>/` would, except that the `.js` files must use the exact version of the package: ``` node_modules/underscore/ flow-typed/ flow_v0.13.x-v0.37.x/ underscore_v1.8.3.js flow_v0.38.x-/ underscore_v1.8.3.js ```

jedwards1211 · 2017-12-04T22:26:39Z

@gantoine are you able to run the chrome node inspector on Jest? I'm seeing errors like "Unable to open devtools socket: address already in use."

…ests

gantoine · 2017-12-04T23:15:56Z

@jedwards1211 I've never been able to use the debugger successfully, I usually resort to console.log.

jedwards1211 · 2017-12-04T23:28:19Z

@gantoine it was painful but I finally solved my issues with console.log. I just pushed changes: now this branch looks for the flow-self-typed directory in an installed package, and the tests are now correct.

Notes:

I had to add a selfTyped flag to self-provided NpmLibDefs to handle two edge cases:
- If you have underscore^1.6.0 in your dependencies, flow-typed will not install defs for 1.8.3 (for example) from the central repo. But if you have 1.8.3 installed and it provides its own types (at node_modules/underscore/flow-self-typed/flow_v0.38.x-/underscore_v1.8.3.js), they will get installed as flow_typed/npm/underscore_v1.8.3.js.
- In getNpmLibDefVersionHash, instead of trying to get a git commit hash for self-provided type defs, I use the library package version instead.

gantoine added bug enhancement An addition to an existing component and removed bug labels Oct 31, 2017

gantoine added the wishlist label Nov 3, 2017

jedwards1211 mentioned this pull request Nov 8, 2017

Roadmap to a v3.0.0 of flow-typed #1494

Open

gantoine changed the title ~~feature: install defs from node_modules/<package>/flow-typed if it exists for each package~~ Install defs from node_modules/<package>/flow-typed if it exists for each package Nov 13, 2017

gantoine changed the title ~~Install defs from node_modules/<package>/flow-typed if it exists for each package~~ Install defs from node_modules/<package>/flow-self-typed if it exists for each package Nov 13, 2017

gantoine added release Related to the next major release and removed enhancement An addition to an existing component labels Nov 13, 2017

gantoine added this to To Do in 3.0.0 Nov 13, 2017

jedwards1211 added 4 commits December 4, 2017 15:20

fix tests by unignorning fake node_modules directories in test fixtures

a18a469

make sure fakeCacheRepo dir for defs-in-pkgs test is committed

0c88879

another fix

0370dc3

fix(flow-self-typed): fix multiple issues and invalid assertions in t…

c1b7c1a

…ests

jedwards1211 force-pushed the flow-typed-in-packages branch from 301c031 to c1b7c1a Compare December 4, 2017 23:11

style(src): fix lint errors [ci skip]

169153e

Merge branch 'master' into flow-typed-in-packages

c70f162

gantoine self-assigned this Dec 6, 2017

gantoine added the cli Related to CLI tool label Mar 21, 2018

gantoine added the on hold label Apr 9, 2018

gantoine removed their assignment Apr 9, 2018

langri-sha mentioned this pull request Dec 22, 2018

Is it currently possible to have dependencies between library definitions? #1857

Closed

villesau assigned gantoine Aug 9, 2019

Brianzchen added this to Todo in Brian's watchlist Jul 23, 2022

Install defs from node_modules/<package>/flow-self-typed if it exists for each package #1435

Are you sure you want to change the base?

Install defs from node_modules/<package>/flow-self-typed if it exists for each package #1435

Conversation

jedwards1211 commented Oct 31, 2017 • edited

Notes

Directory structure

willmruzek commented Oct 31, 2017

jedwards1211 commented Oct 31, 2017

willmruzek commented Oct 31, 2017

jedwards1211 commented Oct 31, 2017 • edited

willmruzek commented Oct 31, 2017

jedwards1211 commented Oct 31, 2017 • edited

willmruzek commented Oct 31, 2017 • edited

jedwards1211 commented Oct 31, 2017

jedwards1211 commented Oct 31, 2017 • edited

willmruzek commented Oct 31, 2017

jedwards1211 commented Oct 31, 2017

willmruzek commented Oct 31, 2017

willmruzek commented Oct 31, 2017

jedwards1211 commented Oct 31, 2017

gantoine commented Nov 1, 2017

jedwards1211 commented Nov 1, 2017

TiddoLangerak commented Nov 1, 2017

jedwards1211 commented Nov 1, 2017

jedwards1211 commented Nov 1, 2017

jedwards1211 commented Nov 2, 2017

gantoine commented Nov 3, 2017

jedwards1211 commented Nov 3, 2017

gantoine commented Nov 3, 2017

willmruzek commented Nov 3, 2017 via email

jedwards1211 commented Nov 3, 2017 • edited

jedwards1211 commented Nov 3, 2017

gantoine commented Nov 5, 2017

chrisgervang commented Nov 6, 2017

chrisgervang commented Nov 6, 2017

jedwards1211 commented Nov 7, 2017 • edited

jedwards1211 commented Nov 29, 2017 • edited

jedwards1211 commented Nov 29, 2017

jedwards1211 commented Dec 4, 2017

gantoine commented Dec 4, 2017

jedwards1211 commented Dec 4, 2017

jedwards1211 commented Oct 31, 2017 •

edited

jedwards1211 commented Oct 31, 2017 •

edited

jedwards1211 commented Oct 31, 2017 •

edited

willmruzek commented Oct 31, 2017 •

edited

jedwards1211 commented Oct 31, 2017 •

edited

jedwards1211 commented Nov 3, 2017 •

edited

jedwards1211 commented Nov 7, 2017 •

edited

jedwards1211 commented Nov 29, 2017 •

edited