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

3.1.0 has broken my typescript build #449

Closed
mikesnare opened this issue Oct 27, 2016 · 14 comments
Closed

3.1.0 has broken my typescript build #449

mikesnare opened this issue Oct 27, 2016 · 14 comments

Comments

@mikesnare
Copy link

Expected behavior:

TSC build still works.

Actual behavior:

TSC build doesn't work.

Since gulp-typescript now includes the @types from required libs, npm install now moves those libs into my node_modules/@types directory and they get picked up by the build. As a result, calls in my code to setTimeout/setInterval and require are failing to compile because of the @types/node typings that get copied up.

Any pointers as to how to prevent this (besides using window.setTimeout, etc...) would be appreciated, but for now I've downgraded to 3.0.2.

@ivogabe
Copy link
Owner

ivogabe commented Oct 28, 2016

@mikesnare It appears that TypeScript will automatically load all @types files, even when they are not a direct dependency. I think that TypeScript should only load the types from direct dependencies. Nested dependencies should only be loaded if a (less deep) dependency refers to it. I've logged microsoft/TypeScript#11917, I hope that they will agree about that. In the meantime you have to set "types": [] to prevent the compiler from loading any @types files. If you do need some other types, you should list all those packages between the brackets.

@ejmarino The package does exist on the npm website, so I don't know why you're seeing that. Are you using a proxy? Or are you using a very old version of Node and NPM?

@felixfbecker
Copy link

felixfbecker commented Oct 28, 2016

This is how it always behaved, typings are always global, you cannot have two versions of the same typings. Especially for ambient typings like node it is not even possible to have conflict resolution - they are global by definition. Installing typings for the user will result in conflicts if the user has typings of the same package installed, which is why I was for #436 (comment). This just broke our builds - in a minor version.

@felixfbecker
Copy link

felixfbecker commented Oct 28, 2016

In my case the error was:

gulpfile.ts (91,15): Argument of type 'Duplex' is not assignable to parameter of type 'WritableStream'

Because I use the better node typings from the typings registry until microsoft/types-publisher#4 is implemented, which conflict with the crappy ones from DT.

@ivogabe
Copy link
Owner

ivogabe commented Oct 28, 2016

In version 3.1.1, @types/node is now marked as a devDependency. So, these types will not be installed any more. I hope to get a response from the TypeScript team soon to see what the best solution is.

@felixfbecker Most @types are actually not global. You can safely install two versions of @types/chalk for instance. Node is the only global type definition that gulp-typescript uses. @demurgos and I asked the team about this, they advised to use a normal dependency and allow a wider range of version to prevent @types/node from being installed twice. Though that didn't prevent these issues.

@felixfbecker
Copy link

typings differentiated between global dependencies and dependencies. Global dependencies always needed to be installed manually.

Personally I will not use @types until I can use the the typings from the registry because the ones on DT are just bad in most cases.

@demurgos
Copy link
Contributor

demurgos commented Oct 28, 2016

@mikesnare: Are you using sources for type declarations such as typings (or tsd) or are you using @types/node ?

I already said it earlier, but I feel that typings was (and still is) the only sane solution to manage type declarations, but @types wins with convenience. For me, @types feels rushed to match the release of TS2 and I'm just sad that it didn't learn from the design of typings:

  • The user controls the environment so he should be the one defining it: only the top project should be able to mess with global declarations.
  • Centralization is not sustainable: DefinitelyTyped is filled with hard to maintain files. People pour a lot of effort in this but you'll still be lagging compared to a dedicated project for each declaration.

With these two factors, we're stuck with leaking and outdated declarations. I was hoping that the first one will be less of an issue if the ease of maintenance (so accuracy of the declarations) is improved but it turns out it won't be so easy.

I have to admit that I was expecting Node's modules to be locally scoped so each lib gets the version of @types/node it needs because npm takes care to install the right version at the right depth. I just checked out it turns out you are right: it is in fact explicitly defining some globals in its types-metadata.json file so it might be hard to contain it. I feel it is like a step backward: as a user I am not expecting my Node's dependencies to have any effect on the outside world (except exposing an entry point). It just reminds me that until TS1.6 (just a few months ago!), the officially recommended way of writing modules was to use "ambient namespaces" instead of "external modules" (it just felt (feels ?) like they were trying to impose a more "Python-like" dependency management).

Here is the answer we got from the type-publishers team about using @types/node. I wasn't that happy but was able to change my project as proposed by @DanielRosenwasser (I had to also update some of my source code), I undestand that it is not always desirable or even possible.
Even if I agree with him that it is more a dependency than a devDependency (or ideally a globalDependency), I think that it should be OK to mark it as a devDependency until the situation is clarified. I'll try to post more feedback about this whole situation on the types-publisher repo.

@mikesnare
Copy link
Author

@demurgos we recently switched to @types/ from typings, but node is not one of our dependencies, hence the errors once 3.1.0 added the node dependency to our @types folder.

@ivogabe I tried 3.1.1 and got the exact same errors regarding the node typings. Backed down to 3.0.2 again.

@demurgos
Copy link
Contributor

demurgos commented Oct 29, 2016

Did you call npm prune after your update to clear no longer needed dependencies ? Was there still a node_modules/@types/node/ directory ?

@felixfbecker
Copy link

For users of typings, you can work around by using typeRoots in tsconfig to prefer your own typings.

@ivogabe
Copy link
Owner

ivogabe commented Oct 29, 2016

@mikesnare You probably still have @types/node in your node_modules folder, you can remove that with npm prune (or just remove node_modules and run npm install).

@felixfbecker You shouldn't see this issue any more with 3.1.1, if you still see the same errors you've probably still got @types/node in your node_modules folder. If so, you can remove that with npm prune as @demurgos suggested.

@demurgos If we would have used typings for gulp-typescript, all consumers of the bundled type declarations should install the types for our dependencies manually (either using @types or typings). With @types, these types are automatically installed. The current implementation also doesn't force users to use @types or typings (now that node is a devDependency). Using devDependency for global packages is not absolutely necessary in all cases (as pointed out in microsoft/types-publisher#81), as long as all users are using @types. Though in this case it should be a devDependency.
I see the problem with the centralised repository. However, as long as you're not writing the definitions, @types give a very good user experience in my opinion.

@ivogabe
Copy link
Owner

ivogabe commented Oct 29, 2016

I just saw that @types/vinyl-fs, one of our dependencies, also depends on @types/node. So, now that package causes that the types for Node are included. I looked at the generated declaration files for gulp-typescript, and it appears that the types for all dependencies except TypeScript are not needed there, so it should be safe to mark all of the @types packages as devDependency.

@ivogabe
Copy link
Owner

ivogabe commented Oct 29, 2016

All types packages are now marked as devDependencies. Can everyone verify that the Node types are not installed any more with version 3.1.2? Note that npm install gulp-typescript alone will not uninstall @types/node automatically, so run npm prune afterwards, or remove node_modules before running npm install.

@mikesnare
Copy link
Author

@ivogabe works now. with 3.1.2 my @types dir is only what I put there. Thanks.

@ivogabe ivogabe closed this as completed Nov 3, 2016
@ejmarino
Copy link

ejmarino commented Nov 4, 2016

@ivogabe, In my case it was a mix of things:

  1. I'm using Sinopia Server, that has a problem with '@' character, so now I skipped it.
  2. I configured a dependency with your library with ^ so when I updated npm downloaded this new version asking for @types. Because I was using 'typings', typescript compiler started to scream about duplicates, breaking my build,

Now I moved over to @types but I think it still needs a little polishing.
I have problems with duplicated definition when I link projects

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

5 participants