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 lib definitions getting referenced for projects targeting browser #462

Open
1 of 2 tasks
mashenoy opened this issue Feb 9, 2022 · 7 comments
Open
1 of 2 tasks

Comments

@mashenoy
Copy link

mashenoy commented Feb 9, 2022

Package Version: _______

  • nodejs
    • nodejs version: ______
    • os name/version: _____
  • browser
    • name/version: all
      Describe the bug
      Adding @azure/ms-rest-package automatically adds @types/node type definitions. We are working on a React/Typescript project which targets es5 and adding ms-rest-js reference internally pulls in @types/node which pulls in library definitions for es2018

Due to this typescript compiler does not fail the build if newer ES features are used while targeting older ES runtimes.

Screenshots
Enabling "explainFiles" in typescript configs shows @types/node is added by 'node_modules/@azure/ms-rest-js/es/lib/webResource.d.ts' file.

image
image

To Reproduce

  1. Go to replit (https://replit.com/@mashenoy/ms-rest-js-sample) (You will need to fork the repl)
  2. Run ./node_modules/.bin/tsc -p tsconfig.json in the shell.
  3. @types/node types is added by node_modules/@azure/ms-rest-js/es/lib/webResource.d.ts file (shown by explain files tsc compiler option)

Expected behavior
@azure/ms-rest-js should not pollute lib definitions of consuming applications.

@jeremymeng
Copy link
Member

@mashenoy thank you for reporting the issue. While we are looking into this, I am curious about what you use @azure/ms-rest-js for. Also please note that this library is now in maintenance mode. If you only need the HttpClient, we have created a new package with a lot of improvements: https://www.npmjs.com/package/@azure/core-rest-pipeline

@xirzec
Copy link
Member

xirzec commented Feb 11, 2022

I'm confused about your repro, since it explicitly imports @types/node as part of your app:

image

When I set up a small example project locally that simply uses ms-rest-js and doesn't pull in any types, e.g.

  "dependencies": {
    "@azure/ms-rest-js": "^2.6.0"
  },
  "devDependencies": {
    "typescript": "^4.5.5"
  }

I can use it without dragging in @types/node at all:

import { WebResource, DefaultHttpClient } from "@azure/ms-rest-js";

export async function exampleFunction() {
  const x = new WebResource("https://bing.com");
  const client = new DefaultHttpClient();
  const result = await client.sendRequest(x);
  // is typed as `any`
  return result.readableStreamBody;
}

Also, you said you are targeting ES5, but your repro explicitly targets ES6 and includes that in its lib definitions?

image

@mashenoy
Copy link
Author

mashenoy commented Feb 12, 2022

Apologies ES6/ES5 was typo so please disregard that.

@xirzec: I tried the snippet you shared, If I don't have @types/nodes as part of devDependencies it works fine and lib definitions are not added.

However, let's say @types/node is required by some other dev dependency(build/tests etc.) but not the production code. Here typescript compiler pulls in the lib definitions for node due to the /// <reference types="node" /> in webResource.d.ts even if I target ES5. Is that supposed to happen ?

node_modules/@types/node/index.d.ts
Type library referenced via 'node' from file 'node_modules/@azure/ms-rest-js/es/lib/webResource.d.ts' with packageId '@types/node/[email protected]'

@mashenoy
Copy link
Author

@jeremymeng - Thanks for letting me know on the support for @azure/ms-rest-js.
At the moment, we are only using the HttpClient along side @azure/storage-blob. We will plan to migrate to the @azure/core-rest-pipeline

@xirzec
Copy link
Member

xirzec commented Feb 15, 2022

However, let's say @types/node is required by some other dev dependency(build/tests etc.) but not the production code. Here typescript compiler pulls in the lib definitions for node due to the /// <reference types="node" /> in webResource.d.ts even if I target ES5. Is that supposed to happen ?

Is it pulling that in from transitive dependencies or are you saying for other reasons you need to have your package.json pull in @types/node as a devdep?

Either way, the recommended approach is to manually specify types in your tsconfig.json: https://www.typescriptlang.org/tsconfig/#types

This will give you full control over which types are considered.

@mashenoy
Copy link
Author

@xirzec - Yes that was my understanding as well and I should be able to use types to manually control it. However Triple slash references trump the types setting in the tsconfig.json and the lib definitions for @types/node gets pulled in.

https://replit.com/@mashenoy/typescript-ms-rest-js
Screenshot 2022-02-23 121810

@xirzec
Copy link
Member

xirzec commented Feb 24, 2022

This is surprising to me as well. It seems like there is a very big hammer that might solve this problem:

https://www.typescriptlang.org/tsconfig/#noResolve

The downside is that it disables all triple slash imports, so you end up having to specify everything you need (including module imports, which is unfortunate.)

@DanielRosenwasser is there something obvious that I'm missing here? It feels a little bad that the triple slash gets included in our d.ts even though consumers might not want this typing. I found this open issue: microsoft/TypeScript#33111

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

No branches or pull requests

3 participants