Move @types/node from runtime deps to dev deps

[huntharo]

Problem
Using Enzyme in React Native projects causes TypeScript builds to fail with error:

node_modules/@types/node/index.d.ts(138,13): error TS2300: Duplicate identifier 'require'.
node_modules/@types/react-native/index.d.ts(8598,14): error TS2300: Duplicate identifier 'require'.

Cause
Enzyme lists @types/node as a runtime dependency that gets installed with the NPM package into consuming projects. @types/node conflicts with the types provided in React Native projects and possibly other project types. This runtime dependency should actually be a dev dependency unless there is some legit reason that this dependency needs to be force-installed into consuming projects.

Resolution
Moved @types/node from dependencies to devDependencies.

Dependency Chain

• https://www.npmjs.com/package/enzyme
• https://www.npmjs.com/package/cheerio
• https://www.npmjs.com/package/parse5

[huntharo]

Note: parse5 doesn't seem to use TypeScript at all so I think it might actually be correct to simply remove this @types/node dependency completely. But I need a project maintainer to confirm that. If that's the case I can resubmit a pull request that removes the dependency completely.

[inikulin]

Hi, thank you for the contribution, however I will not accept your PR, since it's an intended behaviour. See #220

[huntharo]

I read the linked issue regarding documentation examples and I do not agree that it indisputably says that the approach taken is correct.

So let's set that aside for the moment and look and the benefits and drawbacks of each approach:

As Dependencies

• Benefits: developers building Node projects would not need to installed Node types when they happen to also depend on parse5 because parse5 installs Node types for them???

Ok let's pause there. Is it really the responsibility of parse5 to install Node types for developers building a package that depends on Node? No. It is not parse5s responsibility to do that. Any developer building a Node project would have installed the Node types long before depending on parse5, if they wanted them.

• Drawbacks: breaks all React Native-dependent projects that have types installed for React Native (requiring manually removing the Node types or finding a way to not depend on parse5).

As devDependencies:

• Benefits: All parse5 developers preserve their ability to use types for Node (but since this doesn't appear to be a TypeScript project I can't see how this is actually a benefit)

• Drawbacks: anyone using Node would have to install Node types unless they are using parse5 which already installs them... which is weird.

Node made the choice to not force-install their types so why is parse5 making that choice for Node?

React Native and Node may be one of the few "core" dependencies where installing types like this is going to be a possibly conflict.

Can you confirm my suspicion that parse5 is not using TypeScript at all? If I'm correct then I would like to politely ask that you just remove references to types because it's causing pain for people that are using TypeScript and it's not helping anything. Thanks for working on your project and contributing to the open source community.

[huntharo]

Additionally, parse5 does NOT depend on Node... so why would parse5 list a dependency on Node types (when it works just fine with React Native).

Please do reconsider this.

[hardcodet]

+1, that just broke TS with React Native for me - I can't follow the rationale; that's what dev dependencies are for, aren't they?

[inikulin]

First of all, @huntharo I'd like to say that I don't appreciate your tone in other thread. I work on this project in my spare time and if I wasn't able to answer you in timely fashion that means that something more important happening in my life at the moment. I'm not your personal support engineer. This is not the way how open source works.

Now, back to the topic. parse5 distributes TypeScript typings, so engineers who use TypeScript as their primary language can start using it without any additional efforts. Since, parse5 inherits from some of node.js built-in classes it requires node.js typings.

Typings are added as npm dependency without fixed version (see: https://github.com/inikulin/parse5/blob/master/package.json#L61). If you install parse5 as a dependency of your project npm will not install it's dev dependencies. This is the reason why node typings are specified as production dependency.

From this perspective nodejs typings is just a subdepency of parse5. It's just a regular npm workflow. I really struggle to understand how it may break any dependent of the package. Is ReactNative uses some specific "magic" ways to manage it's dependencies? If so, the problem definitely shouldn't be addressed in this repo.

[hardcodet]

Hmm, let me make another example: I'm using a lot of dedicated libs when working with ReactNative. Those do have dependencies on RN, but still, they provide typing for their own types, but not the typing of RN. Since I work with RN, I already have those typings at hand anyway.

So if I would be working in a Node project, I'd most likely have those types installed. But I'm not, which is why Parse with its forced import messes up the dev environment for RN developers. Does that make sense?

[inikulin]

So if I would be working in a Node project, I'd most likely have those types installed. But I'm not, which is why Parse with its forced import messes up the dev environment for RN developers. Does that make sense?

Not really. If you already have typings installed npm should peek already installed typings and use it for parse5 (for that reason parse5 has * specified as node typings version).

[fictitious]

The problem is, if you have react-native typings installed in the project where node typings are installed, you get duplicate identifier errors because react-native typings define the same globals as node typings do - require, console and so on.

It does make sense for react-native typings to define these globals because react-native is a separate platform, on the same level as node and browser are platforms. React-native developers do not target node, they don't need node typings because their runtime environment is different, and I can understand why they say that dependency on @types/node messes up their development environment.

The same, just less severe issue happens when you use parse5 in a project that targets browser. Typings for node are pulled in by /// <reference types="node" /> and make all the node stuff available where it shouldn't be.

The workaround I use in my browser projects is to have my own minimal typings for parse5, and force TypeScript to use them via paths in tsconfig.json like this:

  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "parse5": ["./whatever/my-custom-parse5-typings.d.ts"]
    },

[inikulin]

@fictitious Thank you very much for the input. Seems like now I have a full picture of what's going on. However, it's not quite clear what will be a proper solution that will satisfy everyone. As far as I understand, if I remove node typings dependencies RN will still complain about parse5 typings since some of classes exposed by parse5 inherit from streams which are part of node API.

[ljharb]

Complaints about typings are very preferable to making things harder for normal untyped JavaScript users, fwiw (altho obv it’s great if both can be avoided)

[inikulin]

Can someone from folks who works closely with RN give an information if RN provides typings for other node.js APIs like streams? If so, I can make node typings a peerDependency. Otherwise, I don't understand how it's suppose to work, considering that parse5 typings rely on node APIs and it seems like RN compilation will still fail.

[domenic]

I continue to think it's supposed to work with people who use these type-related technologies installing them themselves, separately, and leaving them out of the package that the rest of us consume.

[ljharb]

This is exactly right; it is not the job of a JS package to directly support type systems - the consumer of a JS package should not have to know that type systems even exist at all.

@inikulin type defs should never be a production dependency because type checking is not a runtime feature; it's a build time feature. Type defs are identical in this respect to linter plugins and configs - they should only ever be dev dependencies.

[fictitious]

Otherwise, I don't understand how it's suppose to work

I don't know how to solve a problem when a library needs to have cross-platform typings, and needs to reference things that have potentially different definitions for different platforms. Well I can think of a number of ways, but each would require strictly more long-term effort to maintain.

But the thing is, I suppose the majority of people who are having this issue don't even need to compile anything that is directly or indirectly depends on parse5 - it comes into react-native project as dependency of jest or enzyme which are testing tools, not compile-time dependencies. For them, having @types/node as dev dependency would probably solve their problem. Having it as peer dependency would just result in useless (for them) warnings.

[shepmaster]

Type defs are identical in this respect to linter plugins and configs - they should only ever be dev dependencies.

@ljharb can you help connect the dots between your statement and the ones from the TypeScript developers, who say to not use devDependencies?

[fictitious]

@shepmaster to quote from that thread:

Given that breaking consumers is a worse problem than slightly-larger packages, we've made --save the default in our documentation.

Well there are way more different ways of breaking consumers than typescript developers were able to foresee

[ljharb]

@shepmaster I don't use TypeScript, so I'm not entirely sure what their challenges are. Regardless, this is a JS package on a JS platform in a JS ecosystem, and the concerns of "not JS" continue to need to take a back seat to the concerns of "JS".

[shepmaster]

a library needs to have cross-platform typings, and needs to reference things that have potentially different definitions for different platforms.

It feels like this is the core issue at hand here. From earlier:

some of classes exposed by parse5 inherit from streams which are part of node API.

Using a library that inherits from specific node packages on a different platform (such as React Native) seems like it's going to cause problems whenever you use these types. It feels as if the fact that you are getting "compile time" errors instead of runtime errors is a good thing.

It may be that this library is bigger than it needs to be and the Node-specific stuff should be broken out into a separate package. It may be that there needs to be some abstraction package for the Node-specific and React Native-specific code which this library can then consume.

[shepmaster]

this is a JS package on a JS platform in a JS ecosystem, and the concerns of "not JS" continue to need to take a back seat to the concerns of "JS".

@ljharb as I understand it, this isn't a problem if you aren't using TypeScript, so I think the "on a JS platform in a JS ecosystem" part isn't valid here:

Using Enzyme in React Native projects causes TypeScript builds to fail with error

[inikulin]

Despite me initially trying to follow TypeScript developers suggestion, seeing how many issues it's caused recently I'm starting to think to indeed make node typings a devDependency .

But the problem (if I understand it correct) is that will not fix issue for which this PR was opened: since ReactNative takes in consideration typings of dependencies it will still fail because parse5 provides it's own typings that rely on nodejs typings. (Please, correct me if I'm wrong).
So, the only solution is to remove typings from parse5 completely, which doesn't sound as appealing solution.

This problem is not specific for parse5: there are other packages in npm that provide typings and rely on node typings. My overall impression is that either I'm getting it completely wrong and it would be nice to see comprehensive guidelines from TypeScript folks of how such situation should be handled, or ReactNative does it the wrong way.

Either way, it would be nice to hear opinion of maintainers of these projects. Does anyone care to open issue in appropriate repos, I'm from phone at the mo and already got tired typing this answer =).

[RReverser]

1. It sounds like it's React Native that should be defining their typings that are compatible with Node.js ones.
2. @ljharb It's definitely predominant convention to provide types for any popular libraries alongside the package,as it gives nice completion support for IDEs, support for TypeScript consumers etc. and end user is not supposed to search for and install or require such types separately.
3. The problem is in fact deeper and reveals potential runtime problem too (as any typings definitions do) - parse5 indeed has mode where it relies on Node.js stream module which is the entire reason it depends on Node.js in typings - because calling streaming API of parse5 would fail on React Native in runtime as well if it doesn't provide same stream module (although I never worked with RN and don't know - maybe it does? if so, they should have common typings for such module that would be shared between RN and Node.js official typings)

[ljharb]

@RReverser in my experience, putting types in DefinitelyTyped is the predominant convention for TypeScript; none of the typing systems have a good solution for package authors (that don't use types) to keep their type definitions in sync without having to understand and use the actual type system.

[shepmaster]

putting types in DefinitelyTyped is the predominant convention for TypeScript

I believe that DefinitelyTyped is used for augmenting plain JS packages with types when the author doesn't want to maintain them:

When a package bundles its own types, types should be removed from DefinitelyTyped to avoid confusion.

The TypeScript page says:

There are two main ways you can publish your declaration files to npm:

1. bundling with your npm package, or
2. publishing to the @types organization on npm.

If you control the npm package you are publishing declarations for, then the first approach is favored.

[hardcodet]

To be taken with a grain of salt, but: I think the distinction to make is how you treat

• the types for your own package and
• the types of 3rd party packages you depend on (which are dev dependencies)

If a consumer of your package also needs the typings of that 3rd party package, those may be already there, too, or can be imported separately. In any case, it's not your responsibility to provide those (and not desirable either). That's how it seems to be with all the packages I'm currently working with.

[RReverser]

DefinitelyTyped is mostly a legacy way to provide typings where author can't or doesn't want to do so or packages are too old to provide them. It's just a separate Github repo that periodically publishes typings as separate npm packages which consumer would have to install manually; however if it's a modern library, it usually has and is encouraged to have its own typings in the same npm package to reduce bloat and make it more convenient for the end user using any editor with completions or TypeScript.

[RReverser]

Either way, as I said, types in this case, just like usually, expose a problem that would exist at runtime too should you accidentally use an API that depends on Node.js APIs and is not actually compatible with React Native. So it might make sense to either fix RN typings if they provide same API set or maybe split parse5 into engine-agnostic generic parser and another package for streaming one that would continue to depend on Node? Thoughts @inikulin?

[RReverser]

Apparently someone just independently ran into the runtime counterpart of this issue I've been theoretizing about above - see #234 for details.

[inikulin]

Folks I've created an umbrella issue for all the TypeScript/environment related problems: #235 Let's continue discussion there.