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

@types/node should be moved to devDependencies. #711

Closed
k8w opened this issue Mar 21, 2017 · 9 comments
Closed

@types/node should be moved to devDependencies. #711

k8w opened this issue Mar 21, 2017 · 9 comments
Labels

Comments

@k8w
Copy link

k8w commented Mar 21, 2017

protobuf.js version:
6.6.5

Problem
@types/node should be moved to devDependencies in package.json.
For browser project, if there is @types/node, global function like setTimeout setInterval will be judged as wrong type by TypeScript complier.
For example,
setTimeout should return number in browser, but once there is @types/node, the return type is treated as NodeJS.Timer, It may result in many error.

@dcodeIO
Copy link
Member

dcodeIO commented Mar 21, 2017

That's not really an option because when developing with TypeScript, the library's .d.ts files refer to node functionality (i.e. Buffer), even if these (usually transparent and tiny) parts are not used in the browser. One example here is protobuf.BufferWriter vs. protobuf.Writer.

Imho this is rather an issue with the typings in question. not with this library, as it is affecting setTimeout, which is a common / standard function.

@k8w
Copy link
Author

k8w commented Mar 22, 2017

@dcodeIO While I think you should put @types/node into devDependencies.

Because, for @types/node:

  1. Protobuf.JS developer need it.
  2. NodeJS project developer need it, but they would install @types/node by their own.(Because it's NodeJS project!)
  3. Frontend project developer hate it, because it will get some error.
    e.g. the return type of setTimeout and setInterval will be judged as NodeJS.Timer rather than number.
  4. It won't get error without @types/node in a frontend project. (Of course protobufjs installed, I've tried that remove @types/node after install protobufjs)

So devDependencies may be a proper choice.

@dcodeIO
Copy link
Member

dcodeIO commented Mar 22, 2017

Well, I must admit that you have a point with 2. - but this all depends on whether 4. is correct and that there is actually no error without the typings file. Need to test!

@dcodeIO dcodeIO added the unsure label Mar 22, 2017
@dcodeIO
Copy link
Member

dcodeIO commented Mar 22, 2017

Doesn't seem to work without @types/node:

index.d.ts(1196,46): error TS2304: Cannot find name 'Buffer'.
index.d.ts(1332,25): error TS2304: Cannot find name 'Buffer'.
index.d.ts(1339,17): error TS2304: Cannot find name 'Buffer'.
index.d.ts(1347,21): error TS2304: Cannot find name 'Buffer'.
index.d.ts(2206,70): error TS2304: Cannot find name 'Buffer'.
index.d.ts(2838,40): error TS2304: Cannot find name 'Buffer'.
index.d.ts(2846,22): error TS2304: Cannot find name 'Buffer'.

To reproduce:

npm remove @types/node
npm run types

@k8w
Copy link
Author

k8w commented Mar 24, 2017

It seems protobufjs only need Buffer from @types/node.
Prepend a type Buffer = any, may solve this temporarily.

@k8w
Copy link
Author

k8w commented Mar 24, 2017

Well... I know why my frontend project not throw compile error without @types/node..
Because I used const protobufjs = require('protobufjs'). (I have declare var require:any, for webpack)

If I use import * as protobufjs from 'protobufjs' it will compile error....

@dcodeIO
Copy link
Member

dcodeIO commented Mar 24, 2017

See the issue linked above. Basically, if you remove @types/node now, you can use the following reference to fill out the blanks:

/// <reference path="./node_modules/protobufjs/stub-node.d.ts" />

@dcodeIO
Copy link
Member

dcodeIO commented Apr 11, 2017

With the stubs in place, is has ultimately been decided to include @types/node and @types/long as dependencies because this is the intended setup of the npm module.

Hence, custom use cases require custom configuration. Feel free to reopen if necessary, but first contact @microshine as it's all his fault.

@k8w
Copy link
Author

k8w commented Oct 18, 2017

Hi, @dcodeIO
Still don't find a good solution for this.
For protobuf.js is used both in browser and NodeJS project, and @types/node is a dependency.
I have to add rm -rf node_modules/@types/node in postinstall in package.json.

And I have also submit a issue #18588 to TypeScript, hope to have a excludeTypes option.

@mhegazy's reply:

I miss understood the situation then. this seems like a definition issue. include/exclude starts the process. but once there is an import or /// the compiler will suck in the additional definition files. that is the same way include / exclude work today for files as well.

The definition file should not have a dependency on node if it does not need it.

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

No branches or pull requests

2 participants