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

Current version incorrectly analyzes @types/node #20

Open
rmcarias opened this issue Nov 19, 2021 · 32 comments
Open

Current version incorrectly analyzes @types/node #20

rmcarias opened this issue Nov 19, 2021 · 32 comments

Comments

@rmcarias
Copy link

@gustavopch
In my project I have code like this:

export const APP_ENVIRONMENT =
    process.env.ENVIRONMENT 

for which I include in my package JSON @types/node

In previous version (1.1.2) running tsc-file src/*.tsx --noEmit ran correctly without flagging 'process' as an issue (it understood the type).

In latest version: 1.1.3 I now get this:
image

I narrowed it down to this new line you added: (the include) when I comment that out, it works again and TS doesn't flag process as an issue (it sees my types/node)
image

@gustavopch
Copy link
Owner

@rmcarias We're overriding include to an empty array because otherwise all files in include would be processed (see #9 and #5). Do you have any idea on how to fix your issue while not bringing those other issues back?

@rmcarias
Copy link
Author

@gustavopch I didn't get a chance to look into it deeper. What I image could work is to check if they are supplying the path as part of the args and not include the empty array (or add it inside that array):

include:[<whatever_path_was_inputted_from_command_line>]

or else check if they have include already declared on their TS Config and use that?
else empty array?

Not sure if this would work, have not tried it.

@wubenhe
Copy link

wubenhe commented Nov 19, 2021

I think this empty include introduces the issue where custom types are not able to include.
I have below running correctly in v1.1.2, but not in v1.1.3
% yarn tsc-files --noEmit src/App.tsx

src/App.tsx

function App() {
  return (
    <>
          Learn React {`test ${window.test()}`}
    </>
  );
}

export default App;

src/types/window.d.ts

export {};
declare global {
  interface Window {
    test: () => string;
  }
}

@gustavopch
Copy link
Owner

gustavopch commented Nov 20, 2021

@IPWright83 The empty include caused this issue. Do you have an idea about how to fix this without reverting that PR?

@IPWright83
Copy link
Contributor

Hi @rmcarias - this change was designed to deliberately override any include within the tsconfig.json file as otherwise all files are checked, defeating the points of the library. See #5.

What is it exactly that's in your include. I'm unclear as to what's breaking at the moment?

@gustavopch
Copy link
Owner

@IPWright83 Before, global declarations were taken into account because they were included by include. Now they're not. So when TypeScript reads window.test(), it's doesn't know that there's another file that extends the Window type to declare that new method. While we don't want to revert #18, this issue is also pretty bad. I'm still not sure what would be the best way to solve it though.

kylorhall added a commit to kylorhall/tsc-files that referenced this issue Nov 30, 2021
This is an idea to fix gustavopch#20.

Instead of discarding all includes, which seems to discard required global `.d.ts` files, can we use the `typeRoots` a config may have setup instead?

I don't think this quite hits the mark however:
 - Does everyone use `typeRoots` in this manner?  I do not know.
 - Likely will not work for a monorepo with nested tsconfig as files are resolved to the root.
 - Often `typeRoots` will import a fair bit, this could still include a large % of your codebase.  My `typeRoots` includes `node_modules/@types`–this may defeat the purpose gustavopch#18 set out to resolve.
@kylorhall
Copy link

kylorhall commented Nov 30, 2021

While #18 solved one of my issues, I'll have to rollback to a previous version.

I wondered if we could use the compilerOptions.typeRoots on-top of #18? This worked for my two use-cases, but I noted some caveats…idea in a PR: #22

Edit: I also created a minimal example for the above PR: https://github.com/kylorhall/tsc-files-types-example

@IPWright83
Copy link
Contributor

@gustavopch I don't mind if you need to revert.

Honestly I know very little about TypeScript and just hit this rep after discovering terrible performance in our linting (>30s for a single file). I'm hoping to come back and think about this in some more depth soon :)

@kylorhall
Copy link

kylorhall commented Dec 7, 2021

I believe I've sorted the issue for myself in a fairly large codebase (1m LOC, ~200k LOC being Typescript). Just came down to incorrect tsconfig.json configuration.. Maybe this applies to others?

Full findings into: #22 (comment)

I definitely don't think my idea of includes: compilerOptions.typeRoots is a good idea after all that research 😅

@deanolium
Copy link

Is it possible to use a glob in includes to include all *.d.ts files, rather than having it be entirely blank? This is a breaking issue for me so I also had to rollback.

Also, I noticed that 1.1.3 has broken your automated testing, so it really shouldn't have been rolled into your main branch -- it's just bad code practice.

image

@deanolium
Copy link

Maybe a possibility is to have a command line argument to put in files/directories you want to include (so a custom types or declaration directory, or a specific .d.ts file) which you then populate the includes section with? This will reduce the issue of having to parse every file whilst still letting people use their custom declarations.

Or you could check to see if there's an 'tscFilesIncludes` section in the tsconfig.json file, and then populate the includes with that (or empty otherwise). This will let users then decide what files they're including absolutely needs to be processed by tsc-files, without it affecting normal tsc operation:

includes: ['src'],
tscFilesIncludes: ['src/@types']

Though I'm not sure if this might affect any tooling checking for the correctness of tsconfig.json (as we're basically making a new key here)

@gustavopch
Copy link
Owner

@deanolium I don't think a tscFilesIncludes field would be needed as you can already do the same with typeRoots. Please read the comments in #22 and check if it fits your use case.

@deanolium
Copy link

Using a glob could work, reading the Typescript docs. So maybe doing:

includes: ['**/*.d.ts']

or

includes: ['src/**/*.d.ts']

instead of the empty array could be a way to fix the issue without causing the kind of problems people are having.

@deanolium
Copy link

@deanolium I don't think a tscFilesIncludes field would be needed as you can already do the same with typeRoots. Please read the comments in #22 and check if it fits your use case.

My understanding of typeRoots is that it solves a slightly different problem, which is for backwards compatibility with typings. The main issue is that if you leave typeRoots alone, then it will automatically import the types from all node_module directories in your tree. If you put any value into typeRoots, then it won't automatically add any of the node_module types - so you'll need to manually add these. Whilst it would technically work, it isn't really a setting people will be using for TypeScript that often.

Also established packages which a lot of people use (for instance create-react-app with typescript) doesn't set this, so this behaviour will either require people modding their tsconfig. which they might not be knowledgable enough about doing, or convincing these large groups of devs to alter their config. Otherwise you'll essentially end up with people trying your tool, it failing in a way they don't quite understand, and then they just don't bother using it.

Of course, any changes which requires users to alter their config (as I mentioned above) would itself be a pain point and require documentation. So I'm not really sure what the ideal solution is, bearing in mind that a lot of people using this tool probably know enough typescript to develop with it, but not enough to know all the settings in tsconfig and the implications of these changes.

@gustavopch
Copy link
Owner

@deanolium It makes sense. On one hand, we can't simply include everything as it defeats the purpose of the tool (that's what happened before v1.1.3); on the other hand, we need to include global declarations. There's also the case where global declarations happen inside regular .ts files with declare global {}. Adding **/*.d.ts wouldn't help (BTW src/**/*.d.ts wouldn't be good because not everybody places their files inside a src folder). Not sure yet what would be the ideal solution that would cover all possible use cases without defeating the purpose of the tool.

@SpudNyk
Copy link

SpudNyk commented Dec 16, 2021

For the purposes of lint-staged I updated my entry to include my .d.ts files

My package.json went from

{
    "lint-staged": {
        "**/*.{ts,tsx}": [
            "tsc-files --noEmit"
        ]
    }
}

To:

{
    "lint-staged": {
        "**/*.{ts,tsx}": [
            "tsc-files --noEmit src/defines.d.ts src/path/to/other.d.ts"
        ]
    }
}

This worked for my commits as the lint staged files will get added on after.
Maybe an '--global-files' option that supports globing could be added to tsc-files to add be able to use globs in the list as lint staged does not expand them.

@sjungwirth
Copy link

sjungwirth commented Jan 7, 2022

{
    "lint-staged": {
        "**/*.{ts,tsx}": [
            "tsc-files --noEmit src/defines.d.ts src/path/to/other.d.ts"
        ]
    }
}

this seems like the best solution to me. You really only need one file so long as imports whatever other globals you want.

@benwainwrightcinch
Copy link

benwainwrightcinch commented Jan 26, 2022

I discovered today that our large monorepo project that makes use of tsc-files to compile individual files on commit hooks suffered from the problem described in #5.

This can be overcome by creating a base tsconfig containing all your compiler settings, and a different tsconfig for specific use with ts-files, so:

// tsconfig.base.json
{
   // All your typescript settings, WITHOUT your "include", "exclude" or "files" properties
}
// tsconfig.ts-files.json
{
   "extends": "./tsconfig.base.json",
   "include": ["**/*.d.ts"]
}
// tsconfig.json
{
   "extends": "./tsconfig.base.json",
   "include": ["**/*.ts"] // the normal 'include' array for your project
}

This approach works well, and means you are still controlling the compilation source set using the config file rather than the command line as with the above solution. If I were to update the project to the latest version of ts-files, it would break us because you are overwriting the includes property.

In my view, you probably should revert #18, as its an unnecessary solution that can be solved by configuring the compiler properly, and makes your tool harder to understand by those that expect the 'include' property to work as intended by the TypeScript team.

@gustavopch
Copy link
Owner

@benwainwrightcinch Now I'm wondering if an --include option should be added so you'd run tsc-files some-file.ts --include "**/*.d.ts". Perhaps even have **/*.d.ts as the default value? I'd like to know what people following this issue think.

@benwainwrightcinch
Copy link

@gustavopch I think that isn't a bad solution, I think if you do that you should definitely default to include declaration files as you suggest.

@dr-skot
Copy link

dr-skot commented Mar 16, 2022

I can't quite tell what the recommended approach is after reading this thread.

For me tsc-files is choking on on any jest tests that wind up in the staged file list.

Property 'toBe' does not exist on type 'Assertion'.

I've tried

{
  "compilerOptions": {
    "typeRoots": ["./node_modules/@types", "./@types"]
  }
}

but no joy. Do I need to hunt down jest's *.d.ts file somewhere in node_modules and put its path in the typeRoots array?

@gustavopch
Copy link
Owner

@dr-skot Try to run tsc-files file-you-actually-want-to-check.ts globals.d.ts where globals.d.ts is an extra file with the sole purpose of loading Jest types (I believe adding import 'jest' into it would suffice).

@dr-skot
Copy link

dr-skot commented Mar 16, 2022

@gustavopch I'm having success at the moment with

  "lint-staged": {
    "**/*.ts?(x)": "tsc-files --noEmit --pretty next-env.d.ts node_modules/@types/jest/index.d.ts @types/millicast.d.ts"
  }

It doesn't work if I change node_modules/@types/jest/index.d.ts to @types/jest.d.ts where that file's contents are import 'jest'

It would be nice to consolidate that long *.d.ts list into one globals.d.ts... but I don't know how to do that. Do you? This didn't seem to work:

//  globals.d.ts
import 'next-env.d.ts'
import 'node_modules/@types/jest/index.d.ts'
import '@types/millicast.d.ts'

@sjungwirth
Copy link

sjungwirth commented Jun 25, 2022

I'd like to know what people following this issue think.

@gustavopch I'd suggest a warning which offers a recommended solution for projects which define "includes" in tsconfig. As we discussed in #5 it defeats the purpose of the project to include all files.

@dennis-gonzales
Copy link

For the purposes of lint-staged I updated my entry to include my .d.ts files

My package.json went from

{
    "lint-staged": {
        "**/*.{ts,tsx}": [
            "tsc-files --noEmit"
        ]
    }
}

To:

{
    "lint-staged": {
        "**/*.{ts,tsx}": [
            "tsc-files --noEmit src/defines.d.ts src/path/to/other.d.ts"
        ]
    }
}

This worked for my commits as the lint staged files will get added on after. Maybe an '--global-files' option that supports globing could be added to tsc-files to add be able to use globs in the list as lint staged does not expand them.

Props to this comment I fixed the issue on NextJS

"tsc-files --noEmit next-env.d.ts"

@viveleroi
Copy link

If anyone finds a solution for CSS modules/vite environments, please let me know. I don't think I've seen anything in the comment thread here about it but my issue was closed as not-planned, so sounds like we're stuck waiting for tsc to add individual file support.

@gustavopch
Copy link
Owner

gustavopch commented Jan 17, 2023

@viveleroi tsc-files only considers the files that you pass to it, so you must ensure you're passing the file that declares .css modules to it. See #20 (comment).

@slimshreydy
Copy link

slimshreydy commented Apr 5, 2023

@gustavopch I'm having success at the moment with

  "lint-staged": {
    "**/*.ts?(x)": "tsc-files --noEmit --pretty next-env.d.ts node_modules/@types/jest/index.d.ts @types/millicast.d.ts"
  }

It doesn't work if I change node_modules/@types/jest/index.d.ts to @types/jest.d.ts where that file's contents are import 'jest'

It would be nice to consolidate that long *.d.ts list into one globals.d.ts... but I don't know how to do that. Do you? This didn't seem to work:

//  globals.d.ts
import 'next-env.d.ts'
import 'node_modules/@types/jest/index.d.ts'
import '@types/millicast.d.ts'

Would be great to see a mention of this issue in the README! Had a similar issue with next-env.d.ts and environment.d.ts and this solved the issue for me. Figure this will be a common issue for anyone using tsc-files on a Nextjs repo.

@saltedsword
Copy link

"lint-staged": {
    "**/*.{ts,tsx}": "sh -c 'tsc-files --noEmit $(find ./src -name *.d.ts ! -path \"./src/.*/*\") $0 $@'"
},

i'm using this cmd to specify all d.ts

@nikwen
Copy link

nikwen commented Jul 27, 2023

Note for anyone trying to import images into a TypeScript/React project: I had to create separate declaration files for each image type. Putting them into the same file didn't work.

png.d.ts:

declare module "*.png" {
  const content: string;
  export default content;
}

svg.d.ts:

declare module "*.svg" {
  const content: string;
  export default content;
}

@pgross41
Copy link

pgross41 commented Nov 8, 2023

"lint-staged": {
    "**/*.{ts,tsx}": "sh -c 'tsc-files --noEmit $(find ./src -name *.d.ts ! -path \"./src/.*/*\") $0 $@'"
},

i'm using this cmd to specify all d.ts

This seems effective but confusing for people to see in our package.json. Could something like this happen automatically behind the scenes in tsc-files?

@ttys026
Copy link

ttys026 commented Oct 28, 2024

"lint-staged": {
    "**/*.{ts,tsx}": "sh -c 'tsc-files --noEmit $(find ./src -name *.d.ts ! -path \"./src/.*/*\") $0 $@'"
},

i'm using this cmd to specify all d.ts

This seems effective but confusing for people to see in our package.json. Could something like this happen automatically behind the scenes in tsc-files?

A possible less confusing version:

"lint-staged": {
    "**/*.{ts,tsx}": "sh -c 'tsc-files --noEmit $(ls -d ./typings/**/* | grep .d.ts) $0 $@'"
},

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

Successfully merging a pull request may close this issue.