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 #4949

Open
iSuslov opened this issue Feb 15, 2025 · 1 comment
Open

@types/node should be moved to devDependencies #4949

iSuslov opened this issue Feb 15, 2025 · 1 comment
Assignees
Labels
investigate Under investigation and may be a bug. v6 Issues regarding v6

Comments

@iSuslov
Copy link

iSuslov commented Feb 15, 2025

Ethers Version

6.x.x

Search Terms

devDependencies

Describe the Problem

Hey,
This package has @types/node is in dependencies instead of devDependencies. This was apparently added to fix Angular build issues in #3910, but I think that might not be the best solution.

Type definitions are only used during development and compilation - they serve no purpose in production builds. When users install ethers with npm i --omit=dev, they shouldn't need to download these type files.

I get the backwards compatibility concerns (#4893 (comment)) BUT, the errors mentioned in #3910 signaled by typescript and occur during development/build time when devDependencies are available anyway. If a project has issues with types, it's likely a configuration problem in that project that should be fixed upstream, rather than forcing all users to include type definitions in their production builds.

For that Angular project specifically, I tried to run the repo from #3910 (comment)
For that I needed to dedupe some dependencies in package.json and roll back ethers.js to the version prior to the "fix":

 "dependencies": {
    "@angular/animations": "16.0.4",
    "@angular/common": "16.0.4",
    "@angular/core": "16.0.4",
    "@angular/forms": "16.0.4",
    "@angular/platform-browser": "16.0.4",
    "@angular/platform-browser-dynamic": "16.0.4",
    "@angular/router": "16.0.4",
    "ethers": "6.3.0",
    "rxjs": "~7.8.0",
    "tslib": "^2.3.0",
    "zone.js": "~0.13.0"
  },
  "devDependencies": {
    "@angular-devkit/build-angular": "16.0.4",
    "@angular/cli": "16.0.4",
    "@angular/compiler-cli": "16.0.4",
    "@types/jasmine": "~4.3.0",
    "jasmine-core": "~4.6.0",
    "karma": "~6.4.0",
    "karma-chrome-launcher": "~3.2.0",
    "karma-coverage": "~2.2.0",
    "karma-jasmine": "~5.1.0",
    "karma-jasmine-html-reporter": "~2.0.0",
    "typescript": "~5.0.2"
  }

I was able to compile the repo after changing:

  • in tsconfig.json: "moduleResolution": "node16"
  • in package.json: "type": "module"

Proposed solution

I understand that not every project can change moduleResolution to node16 or nodenext so maybe the actual solution should be getting rid of resolution-mode in /// <reference types="node" resolution-mode="require"/> of types/providers/provider-ipcsocket.d.ts?

As per this microsoft/TypeScript#56592 (comment)

resolution-mode="require" only emitted when the file references a global, ambient module

perhaps we can try import from "node:net" here:

import { connect } from "net";
import { SocketProvider } from "./provider-socket.js";
import type { Socket } from "net";
import type { JsonRpcApiProviderOptions } from "./provider-jsonrpc.js";
import type { Networkish } from "./network.js";

@iSuslov iSuslov added investigate Under investigation and may be a bug. v6 Issues regarding v6 labels Feb 15, 2025
@ricmoo
Copy link
Member

ricmoo commented Feb 15, 2025

The test-env CI was introduced in part to check all the possible variations of tweaking the /// <reference blah>, moduleResolution, etc., so if you want to try it out and see if it works, but keep in mind there are 7.2m downloads a month, so breaking backwards compatibility can be tragic and limits a lot of the types of changes we can make to the production library. :s

What is the problem you are having? Keep in mind that types should not introduce any generated code or increase code size, which is why I haven’t considered it a significant issue to resolve.

In the future v7 (which will contain no backwards-breaking changes for the majority of users), the IpcSocketProvider will be a separate package, so this issue goes away. It could also be resolved by providing an inline type definition of the Socket API, but last time I tried that it still had a lot of overhead that needed to be brought in and there was some issue (which I forget now). If it is a problem though, I can re-investigate that solution.

Gourav56-rgb added a commit to Gourav56-rgb/ethers.js that referenced this issue Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Under investigation and may be a bug. v6 Issues regarding v6
Projects
None yet
Development

No branches or pull requests

2 participants