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

Overriding Core Nodejs Module Types #922

Closed
jasonkuhrt opened this issue Oct 9, 2015 · 12 comments
Closed

Overriding Core Nodejs Module Types #922

jasonkuhrt opened this issue Oct 9, 2015 · 12 comments

Comments

@jasonkuhrt
Copy link

I began trying to write types for Node's tls module. This file was within types/tls.js which .flowconfig used under its [libs] block.

However, I was unable to actually leverage my types:

lib/test.js

/* @flow */
import * as TLS from 'tls'



TLS.connect({ host: 'localhost', port: 9000 })

$ flow

lib/test.js:13
 6: TLS.connect({ host: 'localhost', port: 9000 })
        ^^^^^^^ property `connect`. Property not found in
 6: TLS.connect({ host: 'localhost', port: 9000 })
    ^^^ module `tls`


Found 1 error

@jeffmo noted that Flow ships with types for Node's core modules and hence perhaps causing the user's declarations to be somehow overridden.

If @jeffmo is correct then I would suggest a patch that inverses this relationship somehow, such that users can have the final say on type declarations. Even if tls was not an empty TODO, a user may wish to update / improve / change the types to their liking. I'm not saying its likely, but it should be possible.

@jeffmo
Copy link
Contributor

jeffmo commented Oct 9, 2015

Indeed this seems to be a conflict between the built-in TODO and the user-defined version here.

We should fix this, but a workaround that might get you unblocked in the meantime might be to copy the built-in flow libs from here in to your own project's library directory, erase (or update) the offending conflicts in your copy of those libs, and then use flow --no-flowlib to omit all built-in libraries when checking.

@jasonkuhrt
Copy link
Author

a workaround that might get you unblocked in the meantime might be to copy the built-in flow libs from here in to your own project's library directory, erase (or update) the offending conflicts in your copy of those libs, and then use flow --no-flowlib to omit all built-in libraries when checking.

@jeffmo Thanks for this stopgap. I admit it made me wince : ) but such is life. I will report back upon trying.

@samwgoldman
Copy link
Member

I'm reminded of an old XP maxim, "if something is hard, do it more often." I think it's time we removed the dom, node, and react libs out of the default flowlib and just keep core. We'll be forcing users to pull in the libs they need, so we need to find some way to make that less painful.

We need to do it eventually, because APIs change—React 0.14 is upon us, and it should have it's own separate declaration file. Node's API will change, too. We might want to have separate declaration files for different versions of the DOM API, too. Making this stuff opt-in will open the door to all that, we just need to make it convenient.

@ssorallen
Copy link
Contributor

@samwgoldman 👍 I just ran into a function that is present in Node v0.12 but gone in v4.1 and was curious about the future of the lib files.

Do you see Flow maintaining and shipping these libraries still?

@jeffmo
Copy link
Contributor

jeffmo commented Oct 9, 2015

@samwgoldman: For things like React, yea that's ultimately the goal. We need a way for those libs to ship their own interface definitions first, though (DefinitelyTyped really isn't ideal because it isn't coupled/versioned alongside the library itself...). Fortunately @gabelevi is working on this featureset (ability to publish interface defs alongside a library and have Flow understand this trivially).

As for libdefs for various common environments (Node0.12, Node0.14, DOM, etc) -- I actually thing we should include these with Flow and just allow the user to specify which of them they want (opt-in?) in their .flowconfig options. These are just so pervasive there's very little chance that you want neither of them in most projects.

In any case, though, we should probably warn if a user's lib defs collide with some built-ins.

@samwgoldman
Copy link
Member

I agree that we can continue to include the node/dom lib defs with Flow. We just shouldn't load them by default, and ideally we should provide versions as you hinted at. I agree that .flowconfig is a reasonable place for that.

@jasonkuhrt
Copy link
Author

@jeffmo et al: How does one start flow (server) with --no-flowlib? I see flow check --no-flowlib but not flow --no-flowlib.

––Edit: Answer: flow start --no-flowlib.

@henridf
Copy link
Contributor

henridf commented Dec 23, 2015

I was wondering about moving the node libdefs out of flow and found my way to this issue.

It would be nice if there was an easy workflow for importing various external lib def(s) as npm package(s) and configuring their use in a project. Babel's plugin's and presets come to mind here. This would make it easier to have different libdef versions matching different underlying versions (node 0.12 vs 4.x vs 5.x) and would enable competing libdefs with different approaches (more/less strict).

@unscriptable
Copy link

We hit a similar issue. The frustrating part is that flow is silent about the module name conflicts. We found out the hard way that flow is ignoring our type definitions in [libs] when there is a module with the same name (but no flow types) in /node_modules/.

Flow should be very vocal about ambiguities such as this.

@unscriptable
Copy link

Looks like my previous comment is being handled under #676 :)

@brainlock
Copy link

In case anybody finds this issue, the workaround that I'm using until there's a real solution for this:

a file without the // @flow annotation, say src/unchecked.js:

export { mkdtempSync } from 'fs';

then I can import from there:

import { mkdtempSync } from './unchecked';

//...
let tmpdir = mkdtempSync(prefix);

Hope it helps.

@calebmer
Copy link
Contributor

If you want to change the Node.js type definitions you can contribute to Flow or use Flow without the builtin libdefs. We hope to move the builtin libdefs to flow-typed at some point where they will be easier to contribute to 😊

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

No branches or pull requests

8 participants