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

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

Closed
zaynv opened this issue Feb 18, 2018 · 43 comments
Closed
Labels
cli Related to CLI tool question release Related to the next major release

Comments

@zaynv
Copy link

zaynv commented Feb 18, 2018

I am pretty confused about this. Issues like #16 and #1494 seem to suggest that this isn't currently doable, but definitions like react-redux seem to be doing it. Is redux a special case or something that allows for this, or is this actually possible for any library?

@EdwardDrapkin
Copy link
Contributor

AIUI, it's not currently possible.

react-redux seems to only be using React$Component and React$Element which are shipped with Flow itself, e.g. https://github.com/facebook/flow/blob/v0.66.0/lib/react.js#L26

There's a lot more shipped inside Flow itself than you might expect, so I always send my developers a link to the cheatsheet which is programmatically generated from source so it should be complete.

@zaynv
Copy link
Author

zaynv commented Feb 21, 2018

@AndrewSouthpaw
Copy link
Contributor

Hmm, I'm not sure, but my guess would be it's possible, especially since it seems to be working in the case of react-redux. It could also create a snarled, complicated mess of interdependencies (which quickly turns into circular ones). What's your use case?

@saadq
Copy link
Contributor

saadq commented Feb 22, 2018

I'm actually starting to think this isn't currently possible, and the libdefs that are currently doing it right now like react-redux, body-parser, etc might actually just be incorrect/broken.

I tried out an extremely simple case by creating a fake module in the npm/definitions called fakemodule, and just having this:

fakemodule_v1.x.x.js

import type { Dispatch } from "redux";

declare module "fakemodule" {
  declare export var thing: Dispatch;
}

test_fakemodule_v1.x.x.js

import { thing } from "fakemodule";

// This stuff shouldn't pass, but it does
(thing: string);
(thing: number);
(thing: boolean);

// Just to prove tests are actually running
const num: number = '10';

The only error message that appears is about num being an incorrect type.

screen shot 2018-02-22 at 8 39 13 am

screen shot 2018-02-22 at 8 23 47 am

It seems to be giving thing a type of any. Just to mess around with it some more, I tried various things like changing import type { Dispatch } from "redux"; to import type { Dispach } from "redux"; and it didn't complain about that either.

I'm pretty sure that having any type of imports of other libdef types just gives those types a value of any, regardless of what it is and whether it's a valid import or not.

I had also tried this out before for a personal use case where I was trying to import types from koa to use in other libdefs like koa-router, koa-bodyparser, etc. Whenever I tried doing something like import type { Middleware } from 'koa';, the Middleware type seemed to just be getting a type of any.

TLDR:

I don't think it is currently possible to have dependencies between library definitions. flow-typed will not complain if you attempt to do so, but it won't actually be working properly (everything you import will just be given a type of any).

@stereobooster
Copy link

@saadq maybe // @flow in the beginning of files will help?

Related: reduxjs/react-redux#880

@saadq
Copy link
Contributor

saadq commented Feb 26, 2018

Also just found #1795 which seems to describe the current situation with using import.

@villesau
Copy link
Member

villesau commented Mar 7, 2018

There are two different ways of importing in libdefs:

import type { MyType } from 'some-module';
declare module 'other-module' {
  declare export function takesMyType(val: MyType): number;
}

and the second is this:

declare module 'other-module' {
  import type { MyType } from 'some-module';
  declare export function takesMyType(val: MyType): number;
}

How are these different from each others? The first one does not work and returns any type, the second one works. Also, AFAIK second approach only supports importing types, so e.g classes etc are tricky. I haven't tried if you can import class on the top of the file and use it's type with typeof, it might also just turn out to be any.

Unfortunately flow-typed does not currently support the second approach as it can't depend on other libdefs during the tests. Other problem is nasty dependencies between library definitions which might become very hard to maintain.

Ideally flow-typed would support dependencies between libdefs, and in optimal situation flow-typed install module@1.2.3 would also install possible dependencies.

@sobolevn
Copy link
Contributor

sobolevn commented May 3, 2018

@AndrewSouthpaw isn't this issue a duplicate of #16 ?

@AndrewSouthpaw
Copy link
Contributor

Nicely spotted. Seems that way, yeah.

@langri-sha
Copy link
Contributor

Are there any recommendations on what we can do in the meantime? The options that come to mind are to either use any or duplicate declarations between library definitions. Both seem like a heavy choice to make.

@saadq
Copy link
Contributor

saadq commented Dec 21, 2018

There is one alternative solution which is to just download most of the type definitions from flow-typed, and any definitions that need to have dependencies on other definitions need to be written within your own project. From your own project, you CAN import types from other libraries.

For example, I have a project like so:

.
├── .flowconfig
├── flow-typed
│   └── npm
│       └── koa_v2.x.x.js
├── index.js
├── custom-libs
│   └── koa-router.js
├── package-lock.json
└── package.json

From within koa-router.js, I actually am able to do something like import type { Context } from 'koa' inside a declare module. You can see the full source code here:


koa-router.js
// @flow

declare module "koa-router" {
  import type { Context } from "koa";

  declare type KoaRouter$Middleware = (
    ctx: Context,
    next: () => Promise<void>
  ) => Promise<void> | void;

  declare type KoaRouter$ParamMiddleware = (
    param: string,
    ctx: Context,
    next: () => Promise<void>
  ) => Promise<void> | void;

  declare class Router {
    constructor(opts?: {
      prefix?: string,
      sensitive?: boolean,
      strict?: boolean,
      methods?: Array<string>
    }): Router;

    get(
      name: string,
      route: string | string[],
      handler: KoaRouter$Middleware
    ): this;
    get(
      route: string | string[],
      ...middleware: Array<KoaRouter$Middleware>
    ): this;

    patch(
      name: string,
      route: string | string[],
      handler: KoaRouter$Middleware
    ): this;
    patch(
      route: string | string[],
      ...middleware: Array<KoaRouter$Middleware>
    ): this;

    post(
      name: string,
      route: string | string[],
      handler: KoaRouter$Middleware
    ): this;
    post(
      route: string | string[],
      ...middleware: Array<KoaRouter$Middleware>
    ): this;

    put(
      name: string,
      route: string | string[],
      handler: KoaRouter$Middleware
    ): this;
    put(
      route: string | string[],
      ...middleware: Array<KoaRouter$Middleware>
    ): this;

    delete(
      name: string,
      route: string | string[],
      handler: KoaRouter$Middleware
    ): this;
    delete(
      route: string | string[],
      ...middleware: Array<KoaRouter$Middleware>
    ): this;

    del(
      name: string,
      route: string | string[],
      handler: KoaRouter$Middleware
    ): this;
    del(
      route: string | string[],
      ...middleware: Array<KoaRouter$Middleware>
    ): this;

    all(
      name: string,
      route: string | string[],
      handler: KoaRouter$Middleware
    ): this;
    all(
      route: string | string[],
      ...middleware: Array<KoaRouter$Middleware>
    ): this;

    use(...middleware: Array<KoaRouter$Middleware>): this;
    use(
      path: string | Array<string>,
      ...middleware: Array<KoaRouter$Middleware>
    ): this;

    prefix(prefix: string): this;

    routes(): KoaRouter$Middleware;

    allowedMethods(options?: {
      throw?: boolean,
      notImplemented?: () => any,
      methodNotAllowed?: () => any
    }): KoaRouter$Middleware;

    param(param: string, middleware: KoaRouter$ParamMiddleware): this;

    redirect(source: string, destination: string, code?: number): this;

    route(name: string): any | false;

    url(name: string, params?: any): string | Error;

    static url(path: string, params: Object): string;
  }

  declare module.exports: typeof Router;
}
koa_v2.x.x.js
// flow-typed signature: 225656ba2479b8c1dd8b10776913e73f
// flow-typed version: b7d0245d00/koa_v2.x.x/flow_>=v0.47.x

/*
 * Type def from from source code of koa.
 * this: https://github.com/koajs/koa/commit/08eb1a20c3975230aa1fe1c693b0cd1ac7a0752b
 * previous: https://github.com/koajs/koa/commit/fabf5864c6a5dca0782b867a263b1b0825a05bf9
 *
 * Changelog
 * breaking: remove unused app.name
 * breaking: ctx.throw([status], [msg], [properties]) (caused by http-errors (#957) )
**/
declare module 'koa' {
  // Currently, import type doesn't work well ?
  // so copy `Server` from flow/lib/node.js#L820
  declare class Server extends net$Server {
    listen(port?: number, hostname?: string, backlog?: number, callback?: Function): Server,
    listen(path: string, callback?: Function): Server,
    listen(handle: Object, callback?: Function): Server,
    close(callback?: Function): Server,
    maxHeadersCount: number,
    setTimeout(msecs: number, callback: Function): Server,
    timeout: number,
  }
  declare type ServerType = Server;

  declare type JSON = | string | number | boolean | null | JSONObject | JSONArray;
  declare type JSONObject = { [key: string]: JSON };
  declare type JSONArray = Array<JSON>;

  declare type SimpleHeader = {
    'set-cookie'?: Array<string>,
    [key: string]: string,
  };

  declare type RequestJSON = {
    'method': string,
    'url': string,
    'header': SimpleHeader,
  };
  declare type RequestInspect = void|RequestJSON;
  declare type Request = {
    app: Application,
    req: http$IncomingMessage,
    res: http$ServerResponse,
    ctx: Context,
    response: Response,

    fresh: boolean,
    header: SimpleHeader,
    headers: SimpleHeader, // alias as header
    host: string,
    hostname: string,
    href: string,
    idempotent: boolean,
    ip: string,
    ips: string[],
    method: string,
    origin: string,
    originalUrl: string,
    path: string,
    protocol: string,
    query: {[key: string]: string}, // always string
    querystring: string,
    search: string,
    secure: boolean, // Shorthand for ctx.protocol == "https" to check if a request was issued via TLS.
    socket: net$Socket,
    stale: boolean,
    subdomains: string[],
    type: string,
    url: string,

    charset: string|void,
    length: number|void,

//  Those functions comes from https://github.com/jshttp/accepts/blob/master/index.js
//  request.js$L445
//  https://github.com/jshttp/accepts/blob/master/test/type.js
    accepts: ((args: string[]) => string|false)&
    // ToDo: There is an issue https://github.com/facebook/flow/issues/3009
    // if you meet some error here, temporarily add an additional annotation
    // like: `request.accepts((['json', 'text']:Array<string>))` to fix it.
    ((arg: string, ...args: string[]) => string|false) &
    ( () => string[] ) , // return the old value.

//  https://github.com/jshttp/accepts/blob/master/index.js#L153
//  https://github.com/jshttp/accepts/blob/master/test/charset.js
    acceptsCharsets: ( (args: string[]) => buffer$Encoding|false)&
    // ToDo: https://github.com/facebook/flow/issues/3009
    // if you meet some error here, see L70.
    ( (arg: string, ...args: string[]) => buffer$Encoding|false ) &
    ( () => string[] ),

//  https://github.com/jshttp/accepts/blob/master/index.js#L119
//  https://github.com/jshttp/accepts/blob/master/test/encoding.js
    acceptsEncodings: ( (args: string[]) => string|false)&
    // ToDo: https://github.com/facebook/flow/issues/3009
    // if you meet some error here, see L70.
    ( (arg: string, ...args: string[]) => string|false ) &
    ( () => string[] ),

//  https://github.com/jshttp/accepts/blob/master/index.js#L185
//  https://github.com/jshttp/accepts/blob/master/test/language.js
    acceptsLanguages: ( (args: string[]) => string|false) &
    // ToDo: https://github.com/facebook/flow/issues/3009
    // if you meet some error here, see L70.
    ( (arg: string, ...args: string[]) => string|false ) &
    ( () => string[] ),

    get: (field: string) => string,

/* https://github.com/jshttp/type-is/blob/master/test/test.js
* Check if the incoming request contains the "Content-Type"
* header field, and it contains any of the give mime `type`s.
* If there is no request body, `null` is returned.
* If there is no content type, `false` is returned.
* Otherwise, it returns the first `type` that matches.
*/
    is: ( (args: string[]) => null|false|string)&
    ( (arg: string, ...args: string[]) => null|false|string ) &
    ( () => string ), // should return the mime type

    toJSON: () => RequestJSON,
    inspect: () => RequestInspect,

    [key: string]: mixed, // props added by middlewares.
  };

  declare type ResponseJSON = {
    'status': mixed,
    'message': mixed,
    'header': mixed,
  };
  declare type ResponseInspect = {
    'status': mixed,
    'message': mixed,
    'header': mixed,
    'body': mixed,
  };
  declare type Response = {
    app: Application,
    req: http$IncomingMessage,
    res: http$ServerResponse,
    ctx: Context,
    request: Request,

    // docs/api/response.md#L113.
    body: string|Buffer|stream$Stream|Object|Array<mixed>|null, // JSON contains null
    etag: string,
    header: SimpleHeader,
    headers: SimpleHeader, // alias as header
    headerSent: boolean,
    // can be set with string|Date, but get with Date.
    // set lastModified(v: string|Date), // 0.36 doesn't support this.
    lastModified: Date,
    message: string,
    socket: net$Socket,
    status: number,
    type: string,
    writable: boolean,

    // charset: string,  // doesn't find in response.js
    length: number|void,

    append: (field: string, val: string | string[]) => void,
    attachment: (filename?: string) => void,
    get: (field: string) => string,
    // https://github.com/jshttp/type-is/blob/master/test/test.js
    // https://github.com/koajs/koa/blob/v2.x/lib/response.js#L382
    is: ( (arg: string[]) => false|string) &
    ( (arg: string, ...args: string[]) => false|string ) &
    ( () => string ), // should return the mime type
    redirect: (url: string, alt?: string) => void,
    remove: (field: string) => void,
    // https://github.com/koajs/koa/blob/v2.x/lib/response.js#L418
    set: ((field: string, val: string | string[]) => void)&
      ((field: {[key: string]: string | string[]}) => void),

    vary: (field: string) => void,

    // https://github.com/koajs/koa/blob/v2.x/lib/response.js#L519
    toJSON(): ResponseJSON,
    inspect(): ResponseInspect,

    [key: string]: mixed, // props added by middlewares.
  }

  declare type ContextJSON = {
    request: RequestJSON,
    response: ResponseJSON,
    app: ApplicationJSON,
    originalUrl: string,
    req: '<original node req>',
    res: '<original node res>',
    socket: '<original node socket>',
  };
  // https://github.com/pillarjs/cookies
  declare type CookiesSetOptions = {
    domain: string, // domain of the cookie (no default).
    maxAge: number, // milliseconds from Date.now() for expiry
    expires?: Date, //cookie's expiration date (expires at the end of session by default).
    path?: string, //  the path of the cookie (/ by default).
    secure?: boolean, // false by default for HTTP, true by default for HTTPS
    httpOnly?: boolean, //  a boolean indicating whether the cookie is only to be sent over HTTP(S),
    // and not made available to client JavaScript (true by default).
    signed?: boolean, // whether the cookie is to be signed (false by default)
    overwrite?: boolean, //  whether to overwrite previously set cookies of the same name (false by default).
  };
  declare type Cookies = {
    get: (name: string, options?: {signed: boolean}) => string|void,
    set: ((name: string, value: string, options?: CookiesSetOptions) => Context)&
    // delete cookie (an outbound header with an expired date is used.)
    ( (name: string) => Context),
  };
  // The default props of context come from two files
  // `application.createContext` & `context.js`
  declare export type Context = {
    accept: $PropertyType<Request, 'accept'>,
    app: Application,
    cookies: Cookies,
    name?: string, // ?
    originalUrl: string,
    req: http$IncomingMessage,
    request: Request,
    res: http$ServerResponse,
    respond?: boolean, // should not be used, allow bypassing koa application.js#L193
    response: Response,
    state: Object,

    // context.js#L55
    assert: (test: mixed, status: number, message?: string, opts?: mixed) => void,
    // context.js#L107
    // if (!(err instanceof Error)) err = new Error(`non-error thrown: ${err}`);
    onerror: (err?: mixed) => void,
    // context.md#L88
    throw: ( status: number, msg?: string, opts?: Object) => void,
    toJSON(): ContextJSON,
    inspect(): ContextJSON,

    // ToDo: add const for some props,
    // while the `const props` feature of Flow is landing in future
    // cherry pick from response
    attachment: $PropertyType<Response, 'attachment'>,
    redirect: $PropertyType<Response, 'redirect'>,
    remove: $PropertyType<Response, 'remove'>,
    vary: $PropertyType<Response, 'vary'>,
    set: $PropertyType<Response, 'set'>,
    append: $PropertyType<Response, 'append'>,
    flushHeaders: $PropertyType<Response, 'flushHeaders'>,
    status: $PropertyType<Response, 'status'>,
    message: $PropertyType<Response, 'message'>,
    body: $PropertyType<Response, 'body'>,
    length: $PropertyType<Response, 'length'>,
    type: $PropertyType<Response, 'type'>,
    lastModified: $PropertyType<Response, 'lastModified'>,
    etag: $PropertyType<Response, 'etag'>,
    headerSent: $PropertyType<Response, 'headerSent'>,
    writable: $PropertyType<Response, 'writable'>,

    // cherry pick from request
    acceptsLanguages: $PropertyType<Request, 'acceptsLanguages'>,
    acceptsEncodings: $PropertyType<Request, 'acceptsEncodings'>,
    acceptsCharsets: $PropertyType<Request, 'acceptsCharsets'>,
    accepts: $PropertyType<Request, 'accepts'>,
    get: $PropertyType<Request, 'get'>,
    is: $PropertyType<Request, 'is'>,
    querystring: $PropertyType<Request, 'querystring'>,
    idempotent: $PropertyType<Request, 'idempotent'>,
    socket: $PropertyType<Request, 'socket'>,
    search: $PropertyType<Request, 'search'>,
    method: $PropertyType<Request, 'method'>,
    query: $PropertyType<Request, 'query'>,
    path: $PropertyType<Request, 'path'>,
    url: $PropertyType<Request, 'url'>,
    origin: $PropertyType<Request, 'origin'>,
    href: $PropertyType<Request, 'href'>,
    subdomains: $PropertyType<Request, 'subdomains'>,
    protocol: $PropertyType<Request, 'protocol'>,
    host: $PropertyType<Request, 'host'>,
    hostname: $PropertyType<Request, 'hostname'>,
    header: $PropertyType<Request, 'header'>,
    headers: $PropertyType<Request, 'headers'>,
    secure: $PropertyType<Request, 'secure'>,
    stale: $PropertyType<Request, 'stale'>,
    fresh: $PropertyType<Request, 'fresh'>,
    ips: $PropertyType<Request, 'ips'>,
    ip: $PropertyType<Request, 'ip'>,

    [key: string]: any, // props added by middlewares.
  }

  declare export type Middleware =
    (ctx: Context, next: () => Promise<void>) => Promise<void>|void;
  declare type ApplicationJSON = {
    'subdomainOffset': mixed,
    'proxy': mixed,
    'env': string,
  };
  declare class Application extends events$EventEmitter {
    context: Context,
    // request handler for node's native http server.
    callback: () => (req: http$IncomingMessage, res: http$ServerResponse) => void,
    env: string,
    keys?: Array<string>|Object, // https://github.com/crypto-utils/keygrip
    middleware: Array<Middleware>,
    proxy: boolean, // when true proxy header fields will be trusted
    request: Request,
    response: Response,
    server: Server,
    subdomainOffset: number,

    listen: $PropertyType<Server, 'listen'>,
    toJSON(): ApplicationJSON,
    inspect(): ApplicationJSON,
    use(fn: Middleware): this,
  }

  declare module.exports: Class<Application>;
}
index.js
// @flow

import Koa from 'koa'
import Router from 'koa-router'

const app = new Koa()
const router = new Router()

router.get('/', ctx => {
  ctx.body = 'foo'
})

app.use(router.routes())
app.use(router.allowedMethods())

app.listen(3000)

As you can see, this works, and my Context type is properly typed:

screen shot 2018-12-21 at 12 31 36 pm

This wouldn't be the case if I had used the actual koa-router library from flow-typed as it doesn't have access to the types from koa, so it just sets ctx to any.


Obviously this is a pain to do, and can be quite a hassle for larger libraries. Other than this, nothing can really be done until flow-typed makes some changes to how it handles dependencies. The maintainers of flow-typed have been doing an awesome job and I really appreciate the work they've been doing, but there's only a few of them and I don't see this problem being fixed anytime soon.

From what I understand, Facebook itself has pretty much abandoned flow-typed and left it to the community. For the main flow repo, they ignore GitHub issues and they even ignore PRs that the community makes to solves these issues. Microsoft has done a much better job with the TypeScript community and working with DefinitelyTyped. They pay attention to how TypeScript is being used outside of Microsoft and and very responsive in dealing with issues/PRs for both TypeScript and DefinitelyTyped. Facebook seems to primarily focus on Flow problems that they have internally within Facebook and don't pay too much attention to how it is being used outside (they are fully in the right to do so considering it is their project, but you can expect that this will lead to migration from Flow to TypeScript).

Once again, I seriously commend the few people that have been actively maintaining flow-typed on their own time and it makes me sad to say this, but I don't see a future in Flow. The difference in the amount of library definitions is huge, and it is seriously hard to write new definitions for popular libraries (which will obviously have deps on other libs), so that only exacerbates the problem. I used to try to write some Flow library definitions every day and I loved contributing to flow-typed, but honestly it's gotten to the point where it's not worth the effort. It pains me to say this, but I switched to TypeScript awhile back, and my suggestion for others would be to do the same if possible.

@sobolevn
Copy link
Contributor

sobolevn commented Dec 21, 2018

@saadq sorry for the offtopic, but your vscode theme is awesome! What's its name?

@saadq
Copy link
Contributor

saadq commented Dec 22, 2018

It's actually for Atom, it's a custom theme.

UI Theme: https://github.com/saadq/bliss-ui
Syntax Theme: https://github.com/saadq/bliss-syntax

It is based on the popular "Base16 Ocean" theme, so you can just search for that, I'm sure there's some variation of it for VSCode.

@langri-sha
Copy link
Contributor

@saadq this is a nice suggestion! It's a quick and easy way to author library definitions when needed. Unfortunately, when done as such this creates local dependencies which we can't share back to flow-typed. Worth considering is also #1435.

Unfortunately flow-typed does not currently support the second approach as it can't depend on other libdefs during the tests. Other problem is nasty dependencies between library definitions which might become very hard to maintain.

This is a pretty neat and useful containment! It would be nice to find a way how to share library definitions for packages like Apollo, using flow-typed.

@EdwardDrapkin
Copy link
Contributor

EdwardDrapkin commented Dec 22, 2018

I'm here to second what @saadq said. Our next React web project at work is going to be TypeScript so we can evaluate if the React-specific stuff in Flow (e.g. ElementConfig, etc.) outweigh the other benefits that TS brings over Flow. This is the issue that kills Flow adoption every time I try to have a conversation with anyone.

I spent several dozen hours spread across many lunch breaks, weekends, and evenings working on this repository, trying to find a way to fix this issue and open a PR that would at least be a POC that could move the conversation forward, but the code quality of the flow-typed code itself is so (no offense, my apologies) abhorrent that my recommendation (were this a project I was getting paid to work on) would be to greenfield a version 2.0 and manually import code that is still useful going forward. That is an enormous task, however, and it seems like there isn't bandwidth available here in the community to actually get it done. We have over 100 complete, accurate type definition files for various node modules that we can't contribute back upstream because transitive dependencies don't work in flow-typed and it's prohibitively difficult to fix. And that's with two people in the entire department who have the expertise and willingness to actually write libdefs. I shudder to think how many thousands of libdefs a company with actual size would have to be sitting on.

There's nothing that a TS typedef file can define that a Flow libdef cannot also describe AFAIK. If the community wants Flow to succeed, the only way I can see out of this mire is to abandon flow-typed altogether and invest every community effort into making the flow server able to read TS libdef files (or a userspace cross compiler to translate one into the other). Otherwise, the DefinitelyTyped community is way too far ahead of where Flow is, the tooling support in IDEs is much further ahead, and like others have said, Facebook's cold shoulder to the Flow community is as good as a middle finger.

FWIW, I think Flow is a superior product to TS for a multitude of technical reasons, but every other thing about the choice goes to TS right now, and Facebook has not shown that they are willing to fight Microsoft in this cage, so how can anyone in good faith recommend Flow any more? If the Flow community could just absorb the work that the TS community has done - both in typing JS packages and consuming TS code's type information too - that would solve a lot of these issues.

@langri-sha
Copy link
Contributor

It should be possible for someone to publish an NPM package with library definitions that declare types for many related dependencies, unless I'm mistaken. Something that would provide something similar like flow/lib.

But it's much nicer if we can share and collaborate on flow-typed because we all already agree on it. It seems far from ideal if there was a space in the repository where we could contribute library definitions that are opinionated about providing typing for some related packages (which benefit a lot from type imports and reuse), but it does give this value that we can easily discover each other's work.

@langri-sha
Copy link
Contributor

@jedwards1211 there's a more detailed discussion about publishing happening here #3083.

@Brianzchen
Copy link
Member

Have we considered if there's a possibility of a having base/pure type files? Ones that cannot use anything outside of what's defined in flow's lib itself, and then our package defs can only import from these files if they want to import anything. This prevents an issue of circular dependency or a confusing mess of requiring deps from everyone else.

It makes sense as a middle ground between what flow provides which are builtin defs for node/browsers but not libraries, and library defs which should not be reused. This could also be a nice home for React defs some day, given that there's been some chatter about moving react out of flow just like react-dom so that it can be maintained outside of flow versions, but of course if we did that right now it would break everyone using React$AbstractComponent and types like that.

When we install via the cli, flow-typed can handle this in a few ways, maybe it automatically adds all pure defs though that might get very extreme very fast, or it does a dependency tree lookup. Though these are implementation details and it doesn't really matter yet.

Example:
Base file

// using koa as an example because context needs to be reused in middleware
type KoaContext = {
  a: string, // primitives are fine
  b: child_process$spawnSyncOpts, // random type but it's fine because it's a built-in type from flow's node.js
};

Def file A

declare module 'koa' {
  import type { KoaContext } from 'base-file'; // semantics of what module name is can be fixed later

  declare type Middleware = (ctx: KoaContext, next: () => Promise<void>) => Promise<void> | void;
}

Def file B

declare module 'koa-views' {
  import type { KoaContext } from 'base-file'; // semantics of what module name is can be fixed later

  // use same KoaContext instead of currently typing it as any because it would be hard to maintain both Context types
}

App code

// If developer wanted to build their own middleware they could also import types from base files
import type { KoaContext } from 'base-file'; // semantics of what module name is can be fixed later

const customMiddleware = (): KoaContext => { ... }

We could store them in flow-typed/definitions/base and they can be added to a users flow-typed/base along side flow-typed/npm

@jedwards1211
Copy link
Contributor

jedwards1211 commented Dec 28, 2020

If Flow gets the types for imports in our project code from the flow-typed directory, I don't really understand why it can't just use the same code path to resolve types for imports within the flow-typed directory itself. It seems like an architectural defect that could be fixed (I guess no one here knows enough about the guts of Flow though?)

...Though I guess the other problem is that checking defs in this repo is a different situation than a flow-typed directory in a userland project.

@jedwards1211
Copy link
Contributor

Now that I think about it, I wouldn't be surprised if dependencies between defs actually work when they're installed in the flow-typed directory of a userland project, but just don't work within this repo during testing since the folder structure is different.

@Brianzchen
Copy link
Member

They do work when it's in flow-typed/npm dir I've tested this, just as @villesau pointed out above.

But I think the issue is more the dependency management that comes afterwards if this were allowed, if you change a type def, currently we version it against dep version and flow version, but how do we know that won't break a def that imports from this def? And if there's more than one def that relies on the same def of different versions I guess flow would fall apart unless flow can handle multiple files that declare the same module?

Hence the suggestion of base type modules that aren't directly connected a single def, and bind consistency across all defs that need the types of a given flow version. Also since they are structured separately there's a clearer indication that this is used by a lib def and more care is needed as opposed to "I didn't know this was used by anything else".

Testing these should also be possible. As you change these types, the defs that import from them will have their own validation tests

@AndrewSouthpaw
Copy link
Contributor

Sounds like an interesting idea, @Brianzchen. In your example, would the base file be versioned in some way, like /definitions/base/koa_v4.x.x/base.js or something in that vein?

@Brianzchen
Copy link
Member

Kind of, I think something more like /definitions/base/koa/flow_v0.104.x-/base-koa.js since breaking changes in flow versions are the most worrying and everyone being on the latest and greatest can't always be assumed.

The main dirs under base I'm not sure what best convention should be but the general idea is that it's not tied to a particular library so naming could really be anything. One is koa which can share the Context or react which shares react types used in react-redux.

declare module 'base-koa' {
  declare type KoaContext = { ... };
}
declare module 'koa-views' {
  import type { KoaContext } from 'base-koa';

  // ...
}

@Brianzchen
Copy link
Member

Are there any more thoughts around this? This issue has been open for 3 years. We should hopefully be at a level of maturity to at least know what works and doesn't work and decide on a solution that can get us what we want. If we're just looking for the perfect solution we may never get there and need to consider some trade-offs.

As I'm working on more co-dependant libraries I'm feeling the pain ending up with any type. If it's ok with everyone I'd at least be willing to try build a POC of my suggestion and see how it plays out as long as there aren't any glaring holes I haven't noticed yet?

I'm not trying to be pushy but the longer we wait the less useful and nimble flow becomes. With a half decent solution we'd at least be able to clean up so many of the issues that comes with flows eco-system. The tool itself works for me, but the broken or missing types slow us down especially the ones that live in flow and don't get reviewed nearly fast enough to be effective for people who need it. Having a platform to move them out to should work out much better for everyone.

@AndrewSouthpaw
Copy link
Contributor

It seems worth a shot. It's been a minute since I've thought deeply about it, but the only immediate question is "what happens when koa has a major version upgrade that changes all the types"? At first blush, I'm not seeing a place for that to live.

It could also be that you're saying the proposed solution would not catch that, and it's a "better than nothing" solution. I just want to be clear on what we would be accomplishing / leaving out of scope.

@Brianzchen
Copy link
Member

@AndrewSouthpaw I was thinking something like this.

note: I'm prefixing the base def with @flow-typed/base- so that we don't get future conflicts with a real npm lib

declare module '@flow-typed/base-koa' {
  declare type Koa2Middleware = {|
  |};

  declare type Koa3Middleware = {|
  |};
}
declare module 'koa' {
  import type { Koa2Middleware } from '@flow-typed/base-koa';

  declare export type Middleware = Koa2Middleware;

  declare module.exports: () => Koa2Middleware;
}
// @flow
import type { Middleware } from 'koa';

export const myRandomFunc (): Middleware => { ... }

For any reusable types you add it inside an overarching type def that can contain multiple versions of reusable types of a library within the same flow version. This makes it easier to maintain over a long period of time especially if theres a lot of duplication and there was only a minor change between breaking versions.

An important thing to keep in mind is that these defs should not be user installable, and should only be installed as a dependency via a lib def from npm defs. So if I want @flow-typed/base-koa I would install koa which can then import koa@2's middleware type if I need it through koa itself. Although @flow-typed/base-koa would be installed and users can use Koa2Middleware directly or Koa3Middleware instead of Koa2Middleware but at that point it's really up to the users discretion. These @flow-typed/base- modules won't be real npm libraries anyways so they would have really needed to dig into the def files to accidentally use this and maybe at that point they really wanted to if they put that much effort into it.

This is the same idea of using React$Node as opposed to React.Node. But this is only deemed okay because react is shipped directly from flow so we don't risk no one having this def. In the future I guess with this approach it would look something like the following, and React$Node would no longer be a globally usable type.

declare module '@flow-typed/react' {
  declare type React$Node = ...
}

declare module 'react' {
  import type { React$Node } from '@flow-typed/react';

  declare type Node = React$Node;
}

Thinking further I think this solution would continue to work for library authors who ship flowtypes

If I develop a new library called koa-random and I want to ship middleware. I would have koa in my flow-typed which allows me to export Middleware then, as long as I have koa in my peerDependencies my user would know to install koa and when they run flow-typed which I hope everyone uses would provide them koa and @flow-typed/base-koa.


For testing, if I modify base-koa then we'd want to run a backwards lookup for all lib defs that consume this def and run tests on those. Because base defs cannot be directly installed we won't risk breaking consumers code unless tests in flow-typed itself breaks.

Tests from koa for example should adequately cover changes to base-koa. Also if we choose to change the name of a type because we felt like it wasn't named verbosely enough because of version breaking changes as you said, we shouldn't expect consumers to use these directly so we can change them as long as project tests pass.

I guess with that last point, we should probably have these defs prefixed with a user discretion message like These are base definitions that should only be used by lib defs defined inside flow-typed. If you need a type from here they may be exposed from one of your dependencies inside flow-typed/npm

@pascalduez
Copy link
Member

I would need to dig again the problem and re-read that whole thread, but if I'm not mistaken, imports between libdefs works fine when installed in a project in the flow-typed folder? So at the end, it's just how our tests are build and unable to resolve the dependencies?
I'm really not keen about adding more custom entangled files and mechanisms. It's all already not straightforward enough, and we also lack stable maintenance power. It's not the fist time we see people willing to rebuild the whole thing and disappear all of a sudden.

@Brianzchen
Copy link
Member

imports between libdefs works fine when installed in a project in the flow-typed folder?

Yes they do work, the goal I'm trying to express at least is a way have custom reusable types without the likelyhood of a circular dependency and a maintenance nightmare for anyone who didn't implement the original import code.

we also lack stable maintenance power

I agree, we're still no where near close to adding suppression codes everywhere which makes maintenance a very large barrier for defs that don't get maintained much. But counterpoint is that we can't just sit around doing nothing for a problem that is a clear lacking feature in the flow community.

And I'm not saying to rebuild our current solution, I just want to bolt on additional functionality to serve reusable types. I know there's an existing conversation about leveraging mono-repos to publish to npm registry, but personally I think it's quite a far fetched solution that requires way too many libraries to add peerDependencies or something else which won't gain enough traction. I personally like the current solution we have, it's quick, fast, supports constantly flow changes, and requires no effort from library maintainers. Focusing on building a strong third party type library is the only way to gain traction or improve maintenance power from what I can see.

But in the end what are your thoughts on a better way to support this?

@Brianzchen
Copy link
Member

Further thinking, with my current proposal, it keeps the types reusable with exports only within flow-typed. Which means this will never be a solution for core JS definitions such as node.js. I know somewhere someone has requested that there be a way to load either node/web versions of flow as some functions have very different definitions/implementations depending on environment.

But is flow-typed's goal to own and maintain third party lib defs or should it some day be able to own all defs and flow should only own the static analysis engine? Granted it would be quite odd if you install flow-bin for the first time and nothing works without flow-typed installing node env so maybe the answer is no?

@pascalduez
Copy link
Member

But in the end what are your thoughts on a better way to support this?

Out of the blue, without deep thinking, if "all is fine" at the project level, it should only be our test env to evolve and probably do some trickery.

  • Add a bare package.json to liddefs that depends on other libdefs with the other libdefq as dependencies.
  • While launching a test env, read that package.json and build a proper flow env/project/folder with all required libdefs.
  • Launch the tests.

@Brianzchen
Copy link
Member

Do we want to have lib defs depend on each other? @AndrewSouthpaw above mentioned that we don't want potential interdependencies or are we ok with that now? It may become a very careful exercise to determine which def is best to own what and may require breaking refactoring in the future.

@AndrewSouthpaw
Copy link
Contributor

@AndrewSouthpaw I was thinking something like this.

declare module '@flow-typed/base-koa' {
  declare type Koa2Middleware = {|
  |};

  declare type Koa3Middleware = {|
  |};
}

Yeah that holds some potential. If we could establish a pattern of exported types that include at least the major version, that will afford some flexibility to the libdef authors.

The main thing I want to avoid is someone being faced with updating the Koa3Middleware and then it breaking in 10 different places, because of some minor version change.

The other tricky part would be ensuring libdefs stay consistent. If we change Koa3Middleware, then any libdefs that import that type may start failing tests, but right now we wouldn't know about it until the next person tried to update one of those libdefs and ran the tests for it.

So as part of the MVP, I think we should find a way to run those libdef tests as well to ensure no downstream breaking changes.

Overall, I agree with @pascalduez sentiments about lacking consistent maintainer support, and this isn't the first time someone came in with grand ideas about how to fix flow-typed and then ghosted after a couple weeks.

That said, you've been showing up very consistently, and I like that your proposal doesn't involve completely rewriting the whole flow-typed system, it's more of a bolt-on, opt-in system.

Do we want to have lib defs depend on each other?

Ehhhhh I don't know. Your proposal makes libdefs depend on each other, even if it's not happening explicitly. But also to try to make it explicit might be a much bigger challenge and maybe is too much to chew on.

But is flow-typed's goal to own and maintain third party lib defs or should it some day be able to own all defs and flow should only own the static analysis engine?

I don't think we've thought that broadly yet. We've tried to have some conversations with the React Flow team in the past but their responsiveness fluctuates, so we couldn't get into any substantive conversation about where the wind is blowing.

@villesau
Copy link
Member

villesau commented Feb 22, 2021

Hi,

Just my two cents. Unfortunately I no longer use Flow (I wish I would though!) so i have had no bandwidth for this project either :( However, there's bunch of work done in order to migrate the whole project to use NPM:
#3483
and related issue: #3083

With above it would, hopefully, be possible to utilize NPM for dependency resolution.

The PR was never tested thoroughly, but it looked very promising!

@Brianzchen
Copy link
Member

So I started thinking about this change again. My original idea of base types files was getting complicated and needed to have dedicated time to work on which I couldn't afford (making lib defs was less time consuming). So I started considering the dependency of other lib defs (the package.json dependency connection).

I see there are 3 things that would need to work for the package.json change to function properly

  • When launching a test, if there is a package.json it needs to add those referenced defs to the test runner
  • When modifying any lib def, flow-typed would need to scan all package.json's to find out if any lib def is using this lib def and also add their tests to verify nothing between defs have broken
  • When a user installs def A, if it consumes from def B, def B should also be installed (imagine user depends on react-redux but not redux, redux lib def would not be installed for them but consumers should have a seamless experience instead of an error caused by flow-typed)

It's in the last two that I saw a potential hole in the package.json approach. Not sure if it's been considered before or if it's even a problem.

If I wanted to install react-redux, and it had types defined in redux. Which version of redux is installed? Of course we know that flow versions would be aligned, but should redux@4.x.x or redux@3.x.x be installed? You would think redux@4.x.x because it's the latest that but would cause conflicts for anyone who is on redux@3.x.x, the wrong lib def would be installed for a user and they would have duplicated redux modules. Furthermore, if we simply ignore the 3rd point above, redux@3.x.x defs might not be aligned/maintained to expose the right types for react-redux to handle which would cause an issue for flow-typed consumers to handle on their own, needing to check the repo to find out what the right version of redux lib def is necessary for their react-redux to work

This leads me back to the base lib approach which ultimately wouldn't have such a juggling approach, I'd say if base defs are changed, just run the whole test suite on all test(saying that out loud sounds dumb now, considering all the suppression error noise we'd get) we can leverage a package.json that references a base def which doesn't come under the second issue I explored above because there's no versioning involved with base defs besides flow versions.

@Brianzchen
Copy link
Member

^ Hey all , I've taken a stab at enabling cross type dependencies. Would appreciate any feedback and improvements I could do to make flow-typed a better experience!

@Brianzchen
Copy link
Member

#4331
Round 2! Another attempt at dependant definitions, hopefully a more favourable implementation this time around 🙏

Still a WIP but there's enough progress to see how it works and gather potential feedback

@Brianzchen
Copy link
Member

https://github.com/flow-typed/flow-typed#were-migrating-our-mainline-from-master-to-main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to CLI tool question release Related to the next major release
Projects
None yet
Development

Successfully merging a pull request may close this issue.