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

way to extend/override flow/lib types #396

Open
blax opened this issue Apr 17, 2015 · 83 comments
Open

way to extend/override flow/lib types #396

blax opened this issue Apr 17, 2015 · 83 comments

Comments

@blax
Copy link

blax commented Apr 17, 2015

Built-in node.js type declarations are still far from being complete, and filling the missing parts is very cumbersome as Flow doesn't allow to overwrite single modules.

For instance, if I want to fill in some gaps in HTTP types, I have to use flow with --no-flowlib flag, which forces me to copy all the remaining built-in interfaces to my project.

Although such workaround works for now, it's unmaintainable in the longer term, and not obvious to newcomers.

Would it be a problem to make custom declarations override the default ones?

@popham
Copy link
Contributor

popham commented Apr 17, 2015

Why not edit the lib files in place and rebuild Flow? You could pull request your modifications eventually.

@gyzerok
Copy link
Contributor

gyzerok commented Apr 20, 2015

@popham why not to check types using your eyes?

+1 for topic

@nmn
Copy link
Contributor

nmn commented Jul 28, 2015

I propose the following syntax for overriding flow types:

Firstly, we need a way to extend types that are not classes. The syntax is obviously Object spread:

type A = {a: boolean, b: number, c: string}
type B = {...A, d: (...rest: any)=> any}

Next, it should be possible to reassign a declaration:

declare class Document extends Document {
  newProp: boolean;
}
// OR
type Document = {...Document, newProp: boolean}

@popham
Copy link
Contributor

popham commented Jul 28, 2015

What's wrong with class MyDocument extends Document { newProp: boolean }? If your environment has a different Document than Flow's, then why did you use Flow's to begin with?

In these situations, my gut says that you should put the Flow libraries in your project somewhere under version control. When you need to override something, edit it. Any external libraries that you've incorporated into your project will be validated against your environment specification (the version controlled library files). (Corollary: If you're making a library, work against Flow's unaltered libraries.) I guess this could be facilitated by moving Flow's libraries to a separate repo so that users could shallow clone them into projects?

The counterargument goes: "I want my project to see updates to Flow's libraries." I say, "You want your environment specification to change at the whim of Flow's maintainers? You shouldn't." Sure Flow's libraries are still immature, but I think that the proper tact is to help improve them instead of implementing override support (this is low-hanging fruit for non-maintainers). Eventually they'll stabilize and track some ideal environment as it evolves with JS, popularity of Node, popularity of IE8, etc.

(I love the spread syntax, though. Although I would describe it as a non-disjoint union, not as an override.)

@samwgoldman
Copy link
Member

@nmn For your A/B example, you should be able to use intersection types: type B = A & { d: Function } (btw, Function is identical to your (...rest: any) => any type).

@popham
Copy link
Contributor

popham commented Jul 28, 2015

Whoops. Thanks @samwgoldman.

@nmn
Copy link
Contributor

nmn commented Jul 28, 2015

@samwgoldman Thanks for the clarification. I'm still wrapping my head around what intersections do. This makes a ton of sense. So I guess the only part is to be able to override types.

While I don't think there's anything wrong with

class MyDocument extends Document { newProp: boolean }

And I use it already. It would be nice to be able to add/modify properties on a type for specific environments.

By the way, I was wondering if the declare class syntax was pretty much redundant.
For example, the react type definitions have something that looks like this:

declare class ReactComponent<D, P, S> {
  getInitialState(): S;
  componentDidMount(): void;
  ...
}

type ReactClass = Class<ReactComponent>

Is that any different from defining it like so:

type ReactComponent<D, P, S> = {
  getInitialState(): S,
  componentDidMount(): void,
  ...
}

type ReactClass = Class<ReactComponent>

Also, as you mentioned above you can do extends by using intersection.

Just to clarify, I'm not hating on the class syntax (which can be a little confusing), but just trying to understand some of the subtleties of the type system.

@popham
Copy link
Contributor

popham commented Jul 28, 2015

Second part first. The difference is that you cannot extend or new the type ReactComponent version. Flow accepts that the declareed version exists in your JS environment (it's newable and extendable).

First part. The scope of a declare is global, so if you could override, you would be overriding for the entire project. I would argue that as an anti-feature.

  • If it's a class that you want to extend with additional or alternate methods, just extend it and add or override the relevant method.
  • If you disagree with a method's signature (so Flow won't let you override), then use --no-flowlib and provide your own declarations or you can go upstream to Flow's project repo and fix it for the benefit of everybody.

@nmn
Copy link
Contributor

nmn commented Jul 28, 2015

@popham If i'm not wrong, this is what I think: (Second part first)
You can't use the extend keyword but still get the same result:

while you would generally do:

declare class ReactSubComponent extends ReactComponent {
  someProp: any;
}

now you can do:

type ReactSubComponent = ReactComponent & {someProp: any}

As for being able to use the new keyword, instead of using typeof ReactComponent you can use Class<ReactComponent> (I'm not sure about this last part)


Now to the first part. At first I really liked the idea of getting rid of global type definitions.
Though this has some big advantages, this also has the huge downside of adding a ton of work to import types for every file. Project-wide type definitions actually make a lot of sense when you consider cross module inter-op. Having fewer type definitions is always better and will let flow do more type checking for you. So, I'm not sure it's going to be easy to get rid of the global declare method any time soon.

@popham
Copy link
Contributor

popham commented Jul 29, 2015

I'm having trouble understanding you. I've used Flow, I've used React, but I've never used them together, so I'll probably be a little sloppy. The following are coherent, independent narratives:

  • Suppose that I anticipate a bunch of React components that will have a someProp: string property. I'm going to explicate that shared structure by defining a type type ReactSubComponent = ReactComponent & {someProp: string}. Here's a couple of classes that satisfy my ReactSubComponent type:
class MyComponent extends ReactComponent {
  someProp: string;
  constructor() {
    super(...);
    this.someProp = "MyComponent";
  }
}

class YourComponent extends ReactComponent {
  someProp: string;
  constructor() {
    super(...);
    this.someProp = "YourComponent";
  }
}

Now I can create a function that rejects non-ReactComponents and rejects ReactComponents without a someProp: string property:

function printSubComponent(sc: ReactSubComponent): void {
  console.log(sc.someProp);
}

printSubComponent(new MyComponent());
printSubComponent(new YourComponent());
printSubComponent({someProp: "a string"}); // No good.  The object is not an instance of `ReactComponent`.
printSubComponent(new ReactComponent()); // No good.  This instance is missing the `someProp` property.

This subcomponent is just a type. If I try to create an instance, I'll get an error: var noGood1 = new ReactSubComponent();. If I try to extend this type, I'll get an error: class NoGood2 extends ReactSubComponent {...}.

  • Instead, suppose that I have a ReactSubComponent class that extends ReactComponent with a someProp: any property, but Flow doesn't have access to the class:
// Flow doesn't have access to this class.
// It is injected into the JS environment *somehow*.
class ReactSubComponent extends ReactComponent {
  constructor() {
    super(...);
    this.someProp = "ReactSubComponent"; // or `15` or `{randomProperty: ["three"]}`
  }
}

I'm going to attach this class to the global scope of my environment; the identifier ReactSubComponent provides my class from anywhere in my project (as long as it isn't masked). To instruct Flow of my class's existence, I've added a declaration:

declare class ReactSubComponent extends ReactComponent {
  someProp: any;
}

This declaration is not type checked by Flow; it is inside a library file referenced from the .flowconfig file. Now from anywhere in my project (where the identifier isn't masked), I can get a ReactSubComponent:

var sc = new ReactSubComponent();
class MySubSubComponent extends ReactSubComponent {...}

@nmn
Copy link
Contributor

nmn commented Jul 29, 2015

This subcomponent is just a type. If I try to create an instance, I'll get an error: var noGood1 = new ReactSubComponent();. If I try to extend this type, I'll get an error: class NoGood2 extends ReactSubComponent {...}.

You're right about this. ReactSubComponent is just a type so you can't use it in your code at all! but you can use it to annotate classes you may define:

class someClass {...}
var x: ReactSubComponent = someClass

@STRML
Copy link
Contributor

STRML commented Dec 2, 2015

What is the preferred method for dealing with errors like this, when the built-in typedef is just incomplete?

 63:   http.globalAgent.maxSockets = settings.maxSockets;
            ^^^^^^^^^^^ property `globalAgent`. Property not found in
 63:   http.globalAgent.maxSockets = settings.maxSockets;
       ^^^^ module `http`

@nmn
Copy link
Contributor

nmn commented Dec 3, 2015

@STRML that's a problem with the type definitions library. You'll have to manually override the type definition for http.

Maybe something like:

var http: CustomHttpType = require('http');

@STRML
Copy link
Contributor

STRML commented Dec 3, 2015

Is there any way to do it globally, say in an interfaces file, so I don't
have to pollute the code?
On Dec 3, 2015 2:26 AM, "Naman Goel" notifications@github.com wrote:

@STRML https://github.com/STRML that's a problem with the type
definitions library. You'll have to manually override the type definition
for http.

Maybe something like:

var http: CustomHttpType = require('http');


Reply to this email directly or view it on GitHub
#396 (comment).

@phpnode
Copy link
Contributor

phpnode commented Jan 10, 2016

In node it's very common to promisify the fs module with Bluebird, which adds methods like fs.openAsync(), fs.readdirAsync() etc. This pattern happens with a lot of other libraries too. It would be nice to have:

declare module fs extends fs {
  declare function openAsync(filename: string, mode: string): Promise<number>;
}

@STRML
Copy link
Contributor

STRML commented Jan 10, 2016

👍 to this suggestion. It would really help when a definition file is just missing a few methods, rather than having to copy/paste the whole thing from the repo and edit.

@ELLIOTTCABLE
Copy link

ELLIOTTCABLE commented May 15, 2016

👍 here, as well.

My use-case is that I wish to Flow-typecheck my tests (or rather, to flip that on its' head, I wish to typecheck my library, using my tests.); but I always use Chai / should.js style assertions when not targeting ancient engines like IE6.

Tests/giraphe.tests.es6.js:23
 23:          Walker.should.be.a('function')
                     ^^^^^^ property `should`. Property not found in
 28: export { Walker }
              ^^^^^^ statics of class expr `Walker`. See: giraphe.es6.js:28

Basically, while I understand some of the above arguments against such things (hell, this back-and-forth goes back more than a decade, in some ways.), I'd say it boils down to … “Are you really telling me I simply can't use Flow, if my library extends another system's prototypes?” Whether that's extending Object.prototype, testing with should.js-style assertions, using npm modules that extend existing flow-typed libraries with new functionality, or simply dislocating behaviour of a conceptual ‘module’ in your own project across multiple files, using prototype-extension … this happens in the JavaScript world. A lot.

I really, really think Flow should include a mechanism to support it, even if it's felt necessary to add warnings in the documentation that it's considered an anti-pattern, or exceptionally dangerous, or …

@jussi-kalliokoski
Copy link

another reason to consider this is that one could generate flow type definitions from WebIDL (there's a lot of partial stuff in WebIDL)

@cchamberlain
Copy link

Trying to add flow to my library that makes extensive use of chai's should object prototype extension. Is there really no way to extend Object.prototype when chai is imported (via a chai.js module declaration in [libs])?

As @ELLIOTTCABLE mentioned, prototype extension is fundamental and should not make the project incompatible.

These are the types of errors I get:

 41:   initialState.should.be.an('object')
                    ^^^^^^ property `should`. Property not found in
 41:   initialState.should.be.an('object')
       ^^^^^^^^^^^^ object type

@hon2a
Copy link

hon2a commented Nov 10, 2016

@popham (I'm new to Flow, so bear with me.) Seems to me that there's an obvious example of why this is a much needed feature - jQuery plugins. Type definition for jQuery is huge and looks like this:

declare class JQueryStatic {
  (selector: string, context?: Element | JQuery): JQuery;
  // ...
}

declare class JQuery {
  addClass(className: string): JQuery;
  // ...
}

declare var $: JQueryStatic;

When I use a jQuery plugin, e.g. Highcharts, it is added as a new method on the JQuery "class". Its use looks like this:

const chart = $('#myCoolChart', contextNode).highcharts({ ... });

There's no var to re-declare (as shown here), I need to extend the class contract. In this case if I copy the whole type definition into my project and edit it, there's no question of it being useful to anybody else in the future, as you suggested in other scenarios, because it's got nothing to do with jQuery itself. And I do want to get updates of the base definition and it's not an anti-pattern in this case - it's just a clean extension.

@le0nik
Copy link

le0nik commented Nov 27, 2016

Yeap. Decided not to use flow because of issues like this:

element.style.msOverflowStyle = 'scrollbar';

// Error:(12, 15) Flow: property 'msOverflowStyle'. Property not found in CSSStyleDeclaration.

Whenever flow sees property it doesn't know, it throws an error.

Is there a way to work around this?

@vkurchatkin
Copy link
Contributor

@le0nik

(element.style: any).msOverflowStyle = 'scrollbar';

@le0nik
Copy link

le0nik commented Nov 28, 2016

@vkurchatkin thank you, it worked!

@deecewan
Copy link
Contributor

deecewan commented Jun 1, 2018

For things like React, it'd be great to be able to extend the core types temporarily.

For example, react-native is currently at 0.55.4, which has Flow v0.67.1. It does, however, support React 16.3. I'd like to use createRef, which exists in Flow 0.72, but it's not available in 0.67. It'd be great to be able to extend the react module with a single definition for createRef instead of having to copy across the entire react definition from here.

@syrnick
Copy link

syrnick commented Jun 15, 2018

Not sure it's intentional, but this workaround seems to be working for me. Say, I have a case for process.myServiceId = process.env.MY_SERVICE_ID;. I added flow-typed/node.js with this:

class MyProcess extends Process {
    myServiceId: ?string;
}

declare var process: MyProcess;

@STRML
Copy link
Contributor

STRML commented Jun 15, 2018

Yeah, that works well for certain globals. But if you want to e.g. change the type of a field on EventTarget there's no good way that I know of to update it such that it will actually apply in all the many ways you might receive or pass an EventTarget.

@pronebird
Copy link

pronebird commented Jun 19, 2018

I am not able to use chai and chai-spies with Flow for that very reason. Just no way to tell that chai.use(spies) has side effects and which ones.

@magus
Copy link

magus commented Jun 28, 2018

For anyone coming into this looking for a solution, @nmn's post (#396 (comment)) earlier is now possible.

For example,

declare interface Document extends Document {
  mozHidden: boolean,
  msHidden: boolean,
  webkitHidden: boolean,
}

@sobolevn
Copy link

sobolevn commented Jun 28, 2018

That still does not work with custom types.

declare interface Some {
  a: string
}

declare interface Some extends Some {
  b: string
}
5: declare interface Some extends Some {
                     ^ Cannot declare `Some` [1] because the name is already bound.
References:
1: declare interface Some {
                     ^ [1]

https://flow.org/try/#0CYUwxgNghgTiAEBLAdgFxDAZlMCDKA9gLYIDeAUPPFAFzwDOqMKA5uQL7nmiSwIrosOfMQQgAHumTB68QiXgUqAIzqNmyNpyA

@AMongeMoreno
Copy link

Any idea on when (if ever) flow will support extending existing modules? @deecewan makes a good point and now we are facing the same issue with forwardRef for react ...

@sobolevn
Copy link

I came up with the following solution to my Vue problem:

// @flow

import Vue from 'vue'

import type { Axios } from 'axios'
import type { Store } from 'vuex'
import type Router from 'vue-router'

import type { State } from '~/types/vuex'
import type { NuxtAuth } from '~/types/nuxt-auth'

/**
* Represents our extended Vue instance.
*
* We just use the annotations here, since properties are already injected.
* You will need to add new annotations in case you will extend Vue with new
* plugins.
*/
export default class CustomVue extends Vue {
  $auth: NuxtAuth
  $axios: Axios
  $router: Router
  $store: Store<State>
}

And then just using it like so:

// @flow

import Component from 'nuxt-class-component'

import CustomVue from '~/logics/custom-vue'

@Component()
export default class Index extends CustomVue {
  logout () {
    this.$auth.logout()  // type of `logout()` is defined in `NuxtAuth` 
  }
}

And it works!

@AMongeMoreno
Copy link

AMongeMoreno commented Jul 19, 2018

@sobolevn thank you for the suggestion ! The only 'drawback' is now you have to import 'custom-vue' instead of directly importing 'vue', which might (?) cause issues in plugins relying on the import of the actual library.

In my case, I had to deal with 'react' and I think JSX would not be properly understood if I would rename the 'react' library itself.

What I have ended up doing (in case someone else come to this post looking for the same) is to wrap the required function within a 'typed' version of it. Fortunately, in my case the function I was willing to add is forwardRef from React, which is already in the pipeline to be added, so I could refer to the proposed typing from #6103.

In my case the definition looks like:

// $FlowFixMe
import {forwardRef} from 'react';
export function typedForwardRef<Props, ElementType: React$ElementType>(
    render: (props: Props, ref: React$Ref<ElementType>) => React$Node
): React$ComponentType<Props> {
    return forwardRef(render);
}

@YaoHuiJi
Copy link

YaoHuiJi commented Sep 20, 2018

I have meet this issue from the very first beginning of trying to introduce flow into my existing project,and this problem scares me away, and I do not like TypeScript too, so I think maybe I can only keep checking types using my eyes 🤷‍♂️

@skeggse
Copy link
Contributor

skeggse commented Dec 12, 2018

I've got a related use-case: I'd like to force all interactions with DOM APIs to enforce sanitization of untrusted content. I can declare opaque type safeHTMLString = string;, and create APIs that accept a string, process it, and return a safeHTMLString. The missing ingredient is overriding the DOM APIs to only accept safeHTMLString and fail on string. For instance, document.querySelector('body').innerHTML = someStringValue; should fail.

@gdanov
Copy link

gdanov commented Dec 13, 2018

The only solution (actually dirty hack) advised so for (using --no-flowlib) does not work because the modules written this way can't be redistributed.
I think the team owes some explanation to the public why so stubbornly they refuse to move the libs to flow-typed

@sobolevn
Copy link

Maybe the core team members have moved to TypeScript?

@taybin
Copy link

taybin commented Dec 19, 2018

I found creating new types using intersections to be a decent way to extend existing, thirdparty types.

@sijad
Copy link

sijad commented Dec 25, 2018

I was trying to use react hooks in react-native but RN only support flow 0.78 currently, so I can not upgrade flow-bin.
I had to go with @AMongeMoreno solution.

@moll
Copy link

moll commented Sep 10, 2019

I'm looking to create some typings for a testing library of mine, Must.js, that extends Object and other builtins' prototypes. Am I correct in saying that this is still impossible to perform with Flow without copying the entirety of https://github.com/facebook/flow/blob/master/lib/core.js to Must.js and adding a property that way? I do see mentions of intersection types and type extensions, yet if the rest of the codebase is typed to return Object, it doesn't do much if there's also an ExtendedObject that no-one refers to. Thanks!

@hrgdavor
Copy link

Once again after more than a year I am looking into useful way to add strong typing ...

I just don't like TypesScript, and enabling intellisense for plain JS in VS code is not working properly yet
...... more than 4 years since this issue was opened, and nothing ...

@leethree
Copy link

Just try TypeScript. Evidently Microsoft cares more about community than Facebook.

@unscriptable
Copy link

It's now possible to override custom (user-land) types with the spread type operators. This is a huge win for me (finally!). Has anybody tried overriding built-in types this way?

@unscriptable
Copy link

Blog post: https://medium.com/flow-type/spreads-common-errors-fixes-9701012e9d58

@TrySound
Copy link
Contributor

@unscriptable What do you mean by "override custom type"? Spread creates new type like values spread creates new object. There is no mutating happen.

@volkanunsal
Copy link

I think somebody needs to write a blog post about how spreads can be used in place of type extension. I can see a few use cases (listed in this thread) that I can’t figure out how a spread type would help. One way of formulating a motivating use case is this: Given a global type like MouseEvent, how might I use a spread so Flow doesn’t complain when I try to access a new property that is not yet added to the spec definition? And what if the MouseEvent is defined as a class rather than an object?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests