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

skipLibCheck: true is ignored when package's types field points to a .ts file (not a .d.ts declaration file) #41883

Closed
trusktr opened this issue Dec 8, 2020 · 27 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@trusktr
Copy link
Contributor

trusktr commented Dec 8, 2020

TypeScript Version: 3.9.7

Search Terms:

skipLibCheck is ignored
skipLibCheck not working

Code

N/A

Expected behavior:

TypeScript should ignore type errors in node_modules (or skip reporting them? I'm not sure exactly what skipLibCheck does).

Actual behavior:

When skipLibCheck is set to true, TS will still have type errors in packages in node_modules. It seems that this is the case if a package's types field in the package's tsconfig.json file points to a .ts file instead of a .d.ts file.

For example, in my project that consumes the lume package, there are type errors like these:

node_modules/lume/src/html/WithUpdate.ts:132:13 - error TS2784: 'get' and 'set' accessors cannot declare 'this' parameters.

132     get props(this: any): any {
                  ~~~~~~~~~

node_modules/lume/src/core/Utility.ts:5:10 - error TS7030: Not all code paths return a value.

5 function applyCSSLabel(value: any, label: any) {
           ~~~~~~~~~~~~~

As you can see the path in the errors are in node_modules. The lume package's types field points to its src/index.ts file, and hence all the types in that package are based on .ts files instead of .d.ts files.

Playground Link:

N/A


Maybe I misunderstand what skipLibCheck does. What should it do exactly?

@RyanCavanaugh
Copy link
Member

skipLibCheck causes the "check each top-level statement or declaration" step to not occur for .d.ts files.

It has no other effect. It does nothing in .ts files, and is not a guarantee that you won't encounter errors in .d.ts files whose declarations get examined as a part of their resolution.

@trusktr
Copy link
Contributor Author

trusktr commented Dec 8, 2020

I think one would intuitively expect no errors from inside node_modules (unless it is critical, like invalid syntax?) when skipLibCheck is true, because ultimately the dependency code is plain JavaScript (or will end up as plain JavaScript).

For example, the Webpack build in the consumer app works fine, it pulls in the non-.ts files from dist/, and the app works fine.

The types in the lume package work just fine when that repo is cloned and build with tsc. The errors I see are in another project that just installed lume and presumably uses a different version of tsc. Intuition here expects the code to be fine.

How do we assert to tsc that everything is fine and to strictly ignore those type errors in node_modules?

@RyanCavanaugh
Copy link
Member

There isn't a setting that would cause TypeScript to not issue an error in a .ts file

@trusktr
Copy link
Contributor Author

trusktr commented Dec 8, 2020

This circles back to (some issues in) #35822, because we can not consume mixin classes from a package unless the package's types field points to .ts files (because the package author can not output declaration files if they use mixin classes).

@trusktr
Copy link
Contributor Author

trusktr commented Dec 9, 2020

Without types pointing to .ts files, making mixins can go like follows.

From #17744 (comment) (@justinfagnani):

The workarounds we've had to do to satisfy the compiler are pretty onerous. We've created a fake class outside of the mixin and cast the mixin function to return an intersection of the argument and that class. Static require another fake class. Then we need to get our build system to remove the fake class. I'm not sure this approach solved every problem yet.

@DanielRosenwasser DanielRosenwasser added the Working as Intended The behavior described is the intended behavior; this is not a bug label Dec 9, 2020
@DanielRosenwasser
Copy link
Member

We are interested in making .d.ts file generation less of a hassle, but patching skipLibCheck for the use-case of shipping .ts files to get around the issues described is not the direction we want to take.

@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.

@trusktr
Copy link
Contributor Author

trusktr commented Apr 19, 2021

@DanielRosenwasser Is it possible to make TypeScript skip type-checking source files of libraries (i.e. when their package.json types field points to source instead of declaration files)?

This would make shipping neat features possible, that are otherwise currently not allowed when declaration output is turned on.

As an example, all the issues listed in #35822 would be solvable by simply recommending people in those cases to point their types fields to their source entrypoints.

The alternatives are to wait an unknown number of years for a solution, or to abandon the cool features. Adding such a solution would be a very simple unblocker for those nice use cases.

Of course, if declaration output could be fixed, that'd be preferable, but the pace at which that is happening is very slow, and the alternative suggested solution seems to be a lot easier to unblock many people from using cool patterns.

@DanielRosenwasser
Copy link
Member

If the community starts shipping full source in place of .d.ts files, my expectation is that users would start running into scalability concerns in building (and this is actually much of the motivation for project references). It would also potentially exacerbate issues where .d.ts emit would otherwise be stable when adopting new non-declaration ECMAScript features.

@borekb
Copy link

borekb commented May 3, 2021

This issue, together with #38538, is what we're seeing as well.

The core seems to be that that TypeScript checks imported .ts files regardless of whether they are source files or external dependencies, and options like include, exclude or skipLibCheck don't have any effect on this (rightfully so but that doesn't make the issue go away 😄).

We think that TypeScript just crawls imports inside files that match include / exclude and that it then runs a typecheck on everything it finds:

  • .ts files are checked unconditionally
  • .d.ts files are checked if not disabled via skipLibCheck

The first point is a problem – we don't want to see errors from node_modules or generally from .ts files that are not source files of the current project.

I acknowledge that skipLibCheck might not be possible to amend (though intuitively, node_modules/some-plain-ts-library is a library) but there should be some way to prevent it. There is currently no workaround as far as I know.


Some context: besides the use case reported by the OP ("types" pointing to .ts), there's another one.

Modern frameworks like Next.js, Gatsby, CRA and others process TypeScript "natively" so it's actually better not to transpile .ts and leave this job on webpack / Babel / esbuild / whatever the transpilation pipeline that framework uses. This leads to a new world where .js + .d.ts files don't exist on the disk – for example, in our Next.js project, we never run full transpilation, that's up to the next dev / next build to produce some .js bundles.

We're using TypeScript as a "linter" only, i.e., tsc --noEmit.

@RyanCavanaugh
Copy link
Member

The core seems to be that that TypeScript checks imported .ts files regardless of whether they are source files or external dependencies

The first point is a problem – we don't want to see errors from node_modules or generally from .ts files that are not source files of the current project.

This is indistinguishable from an intentional configuration. We'd need some more intentional way for you to specify what's "yours" and "not yours"

@borekb
Copy link

borekb commented May 3, 2021

@RyanCavanaugh I agree.

I intuitively thought that exclude would do this (it seems to be a common try by people hitting this issue) but I now understand that it behaves differently so that's not a way.

I'd say that "my code" is everything under the current folder (where tsconfig.json is) minus node_modules. So <project>/../lib/*.ts is not my source code, neither is <project>/node_modules/abc/index.ts.

I imagine it would still be possible to explicitly include those if one wants, like this:

{
  "include": "node_modules/abc/*.ts"
}

Do you see any glaring holes in this or could it work?

@borekb
Copy link

borekb commented May 3, 2021

Actually, while I assumed that exclude cannot be used by this, is it really that impossible? Would it break some important workflows if exclude started excluding errors from paths under exclude?

(I think I understand what exclude currently does and I know I'm suggesting a change in behavior, I just wonder whether projects realistically list paths under exclude yet expect to see errors from those.)

@KevLehman
Copy link

I faced this same issue past friday. It's even weird because the library we used had a few TS rules off in its package.json, but at the moment it was imported in the project, it ignored the library config and used our main project config, which obviously will fail.

For now, the only workaround we found was to replace the import for that lib with a require.

@ilovejs

This comment has been minimized.

@borekb
Copy link

borekb commented Dec 17, 2021

Tangentially related but Deno 1.17 now supports --no-check=remote:

The --no-check=remote option was added to help with this. When this flag is passed, the program will be type checked as a whole, but any diagnostics that are coming from a remote module will be discarded. If there are no local diagnostics, your program will run.

Feels similar in spirit to this issue.

@ashubham
Copy link

The problem also occurs when the types field in package.json refers to a .d.ts file and there is a .d.ts.map present which links to the .ts source.

Typescript tries to recompile the source in that case, which seems wrong.

@Southclaws
Copy link

Southclaws commented Apr 21, 2022

Just hit this and have no idea how to resolve, no changes to any tsconfig files or updates to typescript or the problematic library but the build is throwing this:

../node_modules/native-base/src/hooks/useContrastText.ts:43:52
Type error: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
  Type 'undefined' is not assignable to type 'string'.

  41 |     bgThemeColorVariant &&
  42 |     themeColorsThresholdShades[bgThemeColorVariant]
> 43 |       ? getContrastThemeColor(bgThemeColorVariant, bgShade)
     |                                                    ^
  44 |       : getAccessibleContrastColor(
  45 |           contrastThreshold,
  46 |           trueDarkText,
info  - Checking validity of types .task: Failed to run task "web-build": exit status 1

What's the advised way to go about this? Surely it can't be changing all imports to requires that would result a huge diff and seems backwards. This is working fine on another branch and it seems to only be this file that's failing.

And yes "skipLibCheck": true, is there and exclude node_modules is there, has been working fine for months.

@ppoulard
Copy link

I have the same issue with "typescript": "^4.7.4"

$ npx tsc --skipLibCheck true
node_modules/react-dropzone-uploader/dist/Dropzone.tsx:504:44 - error TS2571: Object is of type 'unknown'.

504       console.error('Error Upload Params', e.stack)
                                               ~
Found 1 error in node_modules/react-dropzone-uploader/dist/Dropzone.tsx:504

It was working with previous version of typescript (4.5)

@misantronic
Copy link

If you're using webpack with the ForkTsCheckerWebpackPlugin, I found a pretty weird solution which makes the error disappear:

new ForkTsCheckerWebpackPlugin({
    ...,
    issue: {
        include: [
            { file: '**/*' }
        ]
    }
})

So just including everything makes it work for me again.

@melissab1238
Copy link

I faced this same issue past friday. It's even weird because the library we used had a few TS rules off in its package.json, but at the moment it was imported in the project, it ignored the library config and used our main project config, which obviously will fail.

For now, the only workaround we found was to replace the import for that lib with a require.

I changed "module" in "compilerOptions" to be "ES2020" from "commonjs" and it seemed to resolve this bug for me.! excluding 'node_modules', setting skipLibCheck to true, and setting types=[] did not work for me, but this did.

@mwalkerr
Copy link

mwalkerr commented Nov 5, 2022

@misantronic That did it for me, thank you very much for dropping by with that :). How did you end up trying that setting?

@misantronic
Copy link

misantronic commented Nov 5, 2022

That did it for me, thank you very much for dropping by with that :). How did you end up trying that setting?

tbh, I don‘t remember 😅 I might have looked into the source and found how it would work.

@wimbarelds
Copy link

So what is the status here? I understand that all of the existing settings are working as intended (ie: include, exclude, skipLibCheck).

But for the increasingly common use case of using tsc --no-emit for linting purposes (rather than compiling), there doesn't appear to be any workable solution. Should a new issue be made for that? Or is this considered a "wont-fix"?

@deepio
Copy link

deepio commented Oct 6, 2023

For anyone finding this in 2023, using typescript Version 5.2.2 or newer appears to do the right thing when specifying skipLibCheck in the package.json.

@michaelmrn
Copy link

For anyone finding this in 2023, using typescript Version 5.2.2 or newer appears to do the right thing when specifying skipLibCheck in the package.json.

@deepio I'm still observing this with 5.2.2 and skipLibCheck - did you tweak anything else?

@RyanCavanaugh
Copy link
Member

Nothing material about skipLibCheck has changed in 5.2.

skipLibCheck means that .d.ts files aren't "checked" (this is a term of art that means, for each source element, that element is explicitly validated, possibly producing errors; it doesn't mean that an error cannot occur in that file)

There is no setting that causes .ts files to not be checked.

If you have a .ts file in your project where that .ts file isn't under your control (e.g. it's coming from node_modules in someone else's package), that's a configuration error, either on your part or the package author's. Package authors should be configuring their packages such that inbound references resolve to .d.ts files.

There is no setting for ignoring this misconfiguration, since if you're in this state, important compiler settings (exactOptionalPropertyTypes, noUncheckedIndexedAccess, etc) change the meaning of .ts code in ways that will produce incorrect results. The only fix is to fix the misconfiguration.

@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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