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: false does not output diagnostics for ambient types #212

Open
agilgur5 opened this issue Mar 3, 2020 · 8 comments
Open

skipLibCheck: false does not output diagnostics for ambient types #212

agilgur5 opened this issue Mar 3, 2020 · 8 comments
Labels
kind: bug Something isn't working properly priority: backlog Low priority problem: no repro No reproduction was provided (and have not tried to repro without one)

Comments

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 3, 2020

What happens and why it is wrong

tsconfig.compilerOptions.skipLibCheck is by default false, but will always be forced to true when using rpts2.

This isn't documented in the README "Some compiler options are forced" so I'm not sure why this is the case or if this is intentional. I couldn't find any code references or issues about this.

Related to jaredpalmer/tsdx#529 (comment)

Environment

[email protected]

Versions

  • typescript: 3.7.3
  • rollup: 1.27.8
  • rollup-plugin-typescript2: 0.25.3

rollup.config.js

TSDX Rollup

tsconfig.json

package.json

plugin output with verbosity 3

@ezolenko
Copy link
Owner

ezolenko commented Mar 3, 2020

Nothing the plugin does explicitly... Maybe one of the options it does force implies it?

Could you run the build with verbosity 4 and post output, specifically rpt2: built-in options overrides and rpt2: parsed tsconfig sections.

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Mar 4, 2020

Sure, here's verbosity: 3 for rpt2: built-in options overrides and rpt2: parsed tsconfig

Log output
rpt2: built-in options overrides: {
    "noEmitHelpers": false,
    "importHelpers": true,
    "noResolve": false,
    "noEmit": false,
    "inlineSourceMap": false,
    "outDir": "./node_modules/.cache/tsdx/cjs//placeholder",
    "moduleResolution": 2,
    "allowNonTsExtensions": true,
    "declarationDir": "/Users/agilgur5/Desktop/GitHub/react-signature-canvas"
}

rpt2: parsed tsconfig: {
    "options": {
        "sourceMap": true,
        "declaration": true,
        "jsx": 2,
        "module": 99,
        "lib": [
            "lib.dom.d.ts",
            "lib.esnext.d.ts"
        ],
        "importHelpers": true,
        "rootDir": "/Users/agilgur5/Desktop/GitHub/react-signature-canvas",
        "strict": true,
        "noUnusedLocals": true,
        "noUnusedParameters": true,
        "noImplicitReturns": true,
        "noFallthroughCasesInSwitch": true,
        "moduleResolution": 2,
        "baseUrl": "/Users/agilgur5/Desktop/GitHub/react-signature-canvas",
        "paths": {
            "*": [
                "src/*",
                "node_modules/*"
            ]
        },
        "esModuleInterop": true,
        "target": 99,
        "configFilePath": "/Users/agilgur5/Desktop/GitHub/react-signature-canvas/tsconfig.json",
        "noEmitHelpers": false,
        "noResolve": false,
        "noEmit": false,
        "inlineSourceMap": false,
        "outDir": "./node_modules/.cache/tsdx/cjs//placeholder",
        "allowNonTsExtensions": true,
        "declarationDir": "/Users/agilgur5/Desktop/GitHub/react-signature-canvas"
    },
    "fileNames": [
        "/Users/agilgur5/Desktop/GitHub/react-signature-canvas/src/index.tsx",
        "/Users/agilgur5/Desktop/GitHub/react-signature-canvas/typings/trim-canvas.d.ts"
    ],
    "typeAcquisition": {
        "enable": false,
        "include": [],
        "exclude": []
    },
    "raw": {
        "compilerOptions": {
            "sourceMap": true,
            "declaration": true,
            "jsx": "react",
            "module": "esnext",
            "lib": [
                "dom",
                "esnext"
            ],
            "importHelpers": true,
            "rootDir": "./",
            "strict": true,
            "noUnusedLocals": true,
            "noUnusedParameters": true,
            "noImplicitReturns": true,
            "noFallthroughCasesInSwitch": true,
            "moduleResolution": "node",
            "baseUrl": "./",
            "paths": {
                "*": [
                    "src/*",
                    "node_modules/*"
                ]
            },
            "esModuleInterop": true,
            "target": "esnext"
        },
        "include": [
            "src",
            "typings"
        ],
        "compileOnSave": false
    },
    "errors": [],
    "wildcardDirectories": {
        "/Users/agilgur5/Desktop/GitHub/react-signature-canvas/src": 1,
        "/Users/agilgur5/Desktop/GitHub/react-signature-canvas/typings": 1
    },
    "compileOnSave": false,
    "configFileSpecs": {
        "includeSpecs": [
            "src",
            "typings"
        ],
        "validatedIncludeSpecs": [
            "src",
            "typings"
        ],
        "wildcardDirectories": {
            "/Users/agilgur5/Desktop/GitHub/react-signature-canvas/src": 1,
            "/Users/agilgur5/Desktop/GitHub/react-signature-canvas/typings": 1
        }
    }
}

@ezolenko
Copy link
Owner

ezolenko commented Mar 5, 2020

I don't see that option in there. Why do you think it is turned on?

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Mar 6, 2020

If I have a type error in a declaration file, it doesn't error out and the build succeeds. Perhaps "forced to true" is incorrect wording then, instead it doesn't error even if skipLibCheck isn't set.

I can make a repro for this if you need it, just don't have the time right now and figured I'd respond first.

@ezolenko
Copy link
Owner

ezolenko commented Mar 6, 2020

Repro would be good, but no rush on it, I won't be able to fix it right away anyway (unless it is a very easy and safe fix).

PR is even better :)

@agilgur5
Copy link
Collaborator Author

Was debugging something else (.jsx support, c.f. jaredpalmer/tsdx#523) and found that the default exclude filter on the plugin is exclude: [ "*.d.ts", "**/*.d.ts" ] -- I still need to test it out, but I have a feeling that's the problem

@ezolenko
Copy link
Owner

Plugin's exclude is for avoiding transpiling those files (no js would be generated by transpiling d.ts), also typescript files shouldn't be importing type files, I think they are supposed to be magically found by tsc, but never imported.

Also rollup does tree shaking and some files might be never imported. You should see evidence of that in verbose output -- do a build and see if any source files that use affected type file are even transpiled.

Also check if cache is potentially hiding it, do a build with clean: true and see if you see expected error. rpt2 tries to build full import tree when tracking which files should be rebuilt, but ephemeral things like d.ts files might slip through the cracks.

Usually typescript will find all ambient type files and rpt2 will list them at the start of the build and invalidate cache if they changed.

@agilgur5 agilgur5 added the problem: no repro No reproduction was provided (and have not tried to repro without one) label Apr 23, 2022
@agilgur5 agilgur5 changed the title skipLibCheck is forced to true skipLibCheck is forced to true May 22, 2022
@agilgur5 agilgur5 changed the title skipLibCheck is forced to true skipLibCheck: false does not output diagnostics for ambient types Jun 1, 2022
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jun 1, 2022

So now that I know this codebase really well, I'm pretty sure that the problem is that rpt2 doesn't output diagnostics for any ambient types. I've renamed the issue as such.
We basically only output diagnostics in the transform hook, so only for files that are being compiled to JS and not treeshaken by Rollup (i.e. #298)

So while the skipLibCheck setting may impact TS's actual program analysis, no diagnostics will be output regardless of how it's set, because we don't process ambient typings.

Basically, to properly follow TS behavior, we'd want to emit diagnostics for all ambient typings as well when skipLibCheck: false. Possibly in checkAmbientTypes.

That would greatly decrease performance, so I might suggest to add a plugin option that disables this by default. That being said, skipLibCheck is recommended to be false by the TS team anyway:

Definitely low priority if it would be disabled by default; that would just be a tsc parity thing then.

That being said, I believe when I originally made this issue I was actually authoring a declaration file and it wasn't checked (or something? would've been great if I made a repro 😅 ). This not being type-checked because of the default exclude is probably accurate in that case and that use-case wouldn't be so low priority.

@agilgur5 agilgur5 added kind: bug Something isn't working properly priority: backlog Low priority labels Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly priority: backlog Low priority problem: no repro No reproduction was provided (and have not tried to repro without one)
Projects
None yet
Development

No branches or pull requests

2 participants