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

Omitting "dom" from lib should enforce excluding lib.dom.d.ts (for nodejs) #43990

Closed
bluenote10 opened this issue May 7, 2021 · 13 comments
Closed
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@bluenote10
Copy link

bluenote10 commented May 7, 2021

Bug Report

🔎 Search Terms

avoid dom lib, compile without dom, exclude lib.dom.d.ts

Related to:

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about 4.2.3

⏯ Playground Link

Reproducing the problem requires to have dependencies installed, which makes it impossible to reproduce on Playground as far as I can see.

Instead I can offer a self-contained minimal reproduction example here.

💻 Code

In a nodejs backend application, the following code is supposed not to compile:

const time = performance.now();

because in nodejs it requires an explicit import { performance } from "perf_hooks" in contrast to browser code.

The tsconfig.json specifies lib: ["es6"], which according to the docs (and various SO questions) should result in not adding dom types to the compilation.

In addition the project also has npm-installed third party dependencies, in particular @types/superagent.

🙁 Actual behavior

The code compiles successfully.

In fact, we just had our first production backend crash, because one performance.now() slipped through without an explicit import, which clearly should have been caught by the compiler.

🙂 Expected behavior

The code should not compile, as it does before having installed @types/superagent.

Thoughts

The reason why this happens is that the @types/superagent contains a triple slash directive in their type definitions:

/// <reference lib="dom" />

Note: Such problems can also be caused transitively. In our case, the culprit was actually hidden behind npm install --save-dev @types/supertest which transitively brought in the problematic dependency.

Can we simply blame these type definitions and convince the maintainers to "fix" that? Probably not. Having a look at the type definitions, what they do makes sense. The library supports both node and browser contexts, and in a browser context certain types (mainly appearing in union types anyway) are e.g. valid input types. Also, the problem could possibly come up in many more dependencies, i.e., another (future) dependency could break the "strict nodejs types" at any time.

Clearly, third party dependencies should not cause false positive compilations in a non-browser context. Perhaps the most sensible solution would be to consider the /// <reference lib="dom" /> only for the compilation of that particular type definition file, but not to "hoist" it into the compilation of the user code. I.e., if the user omits "DOM" from the lib setting it should be respected for all user code.

@nmain
Copy link

nmain commented May 7, 2021

The flip side is common, and super annoying as well: A browser-only project will include a lib which works in both browser and node, and accordingly includes /// <reference types="node" /> in its own definitions. Then you're getting autocompletions for setImmediate, bad overloads of setTimeout, etc.

See also #18588 #31894

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label May 7, 2021
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented May 7, 2021

There are absolutely a lot of problems in this area that we're trying to fix, but the proposed solution isn't acceptable for a variety of reasons. It'd be a huge breaking change in code that is intentionally set up with lib references in lieu of a tsconfig lib, and breaks the symmetry that a lib reference is the same as a lib specified in config.

Moreover, this doesn't actually fix anything, because as soon as you do this, you'll see compilation errors in the .d.ts files.

Sad to say, but if there were a simple solution like this that actually worked, we'd have done it by now.

@bluenote10
Copy link
Author

bluenote10 commented May 7, 2021

Moreover, this doesn't actually fix anything, because as soon as you do this, you'll see compilation errors in the .d.ts files.

Not if the .d.ts gets type checked in a separate compilation pass, or is using its own "library context". But it is well possible that what I have in mind is practically impossible to implement given how the current compilation context/passes works.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@ab-pm
Copy link

ab-pm commented May 10, 2021

For this particular issue, see DefinitelyTyped/DefinitelyTyped#41425. But I agree it would be good to catch this in the compiler, instead of silently having dom types leak into a project.

@oxc
Copy link

oxc commented May 15, 2021

This can be also very annoying when you have some types with identical names available in your global scope.

In my project, I have a global class Location declared. Pulling in a dependency that enables dom lib breaks compilation.

@leegee
Copy link

leegee commented Aug 19, 2021

This can be also very annoying when you have some types with identical names available in your global scope.

In my project, I have a global class Location declared. Pulling in a dependency that enables dom lib breaks compilation.

Currently facing this when using Express, whose exported Request apparently conflicts with types I did not know were installed until I found this issue.

@dtweedle
Copy link

dtweedle commented Aug 26, 2021

Encountered this in our code base as a result of the @types/superagent package referencing the dom.

We have a frequently used entity called document

We'd expect something like this to fail since the dom lib is not included in our project.

function doSomething() {
  console.log(document) // Should show not defined error
}

This compiles without incident.

@pilaoda
Copy link

pilaoda commented Mar 7, 2023

@RyanCavanaugh Please consider provide config for user to override type reference.
For example in tsconfig.json:

        "compilerOptions": {
		"paths": {
			"@types/node": [
				"empty-types",
			]
		}
	},

@RyanCavanaugh
Copy link
Member

@pilaoda all that's going to do is introduce new errors into your build

@TomJerry56
Copy link

Bug Report

🔎 Search Terms

avoid dom lib, compile without dom, exclude lib.dom.d.ts

Related to:

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about 4.2.3

⏯ Playground Link

Reproducing the problem requires to have dependencies installed, which makes it impossible to reproduce on Playground as far as I can see.

Instead I can offer a self-contained minimal reproduction example here.

💻 Code

In a nodejs backend application, the following code is supposed not to compile:

const time = performance.now();

because in nodejs it requires an explicit import { performance } from "perf_hooks" in contrast to browser code.

The tsconfig.json specifies lib: ["es6"], which according to the docs (and various SO questions) should result in not adding dom types to the compilation.

In addition the project also has npm-installed third party dependencies, in particular @types/superagent.

🙁 Actual behavior

The code compiles successfully.

In fact, we just had our first production backend crash, because one performance.now() slipped through without an explicit import, which clearly should have been caught by the compiler.

🙂 Expected behavior

The code should not compile, as it does before having installed @types/superagent.

Thoughts

The reason why this happens is that the @types/superagent contains a triple slash directive in their type definitions:

/// <reference lib="dom" />

Note: Such problems can also be caused transitively. In our case, the culprit was actually hidden behind npm install --save-dev @types/supertest which transitively brought in the problematic dependency.

Can we simply blame these type definitions and convince the maintainers to "fix" that? Probably not. Having a look at the type definitions, what they do makes sense. The library supports both node and browser contexts, and in a browser context certain types (mainly appearing in union types anyway) are e.g. valid input types. Also, the problem could possibly come up in many more dependencies, i.e., another (future) dependency could break the "strict nodejs types" at any time.

Clearly, third party dependencies should not cause false positive compilations in a non-browser context. Perhaps the most sensible solution would be to consider the /// <reference lib="dom" /> only for the compilation of that particular type definition file, but not to "hoist" it into the compilation of the user code. I.e., if the user omits "DOM" from the lib setting it should be respected for all user code.

With [email protected], Without adding "lib":["dom'] it is throwing error to include dom in lib but on adding lib option in tsconfig is actually causing d.ts errors in my case. Any comments?

@nwalters512
Copy link

Given that we can't tell tsc to omit the DOM types, is there any advice for someone trying to track down why lib.dom.d.ts is ending up in their project? I tried using --traceResolution to see if I could tell what was triggering it to be included, but searching the output didn't yield anything for lib.dom.d.ts.

This is causing a headache for trying to use Node's native fetch(), the types of which conflict slightly with the fetch() declared in the DOM types.

@RyanCavanaugh
Copy link
Member

--explainFiles will tell you the most proximate cause of why a file is being included

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests