Fix compile-ts so it catches typescript errors and add check to CI

[becca-bailey]

In package-scripts.js, there is a nps compile-ts command. However, this isn't currently catching TS build errors, like the one that was created by #2058. We should make sure this compile script shows us real-time TS type errors and add a TS check step to CI.

Steps to reproduce:

1. Uncomment a non-implemented type in victory/src/index.d.ts
2. run nps compile-ts
3. See that there are no errors

[matt-hernandez]

@becca-bailey Did a little poking around. The core of the issue, from what I can tell, is that the typescript compiler doesn't include .d.ts files in the compilation process at all. So a .d.ts file could have any number of errors and the compiler would never complain. I was hoping that ESLint would cover this, but no luck so far.

Did some toying around and a general solution I found was to change packages/victory/src/index.d.ts to packages/victory/src/index.ts and adding these two lines to tsconfig.json

{
  "compilerOptions": {
    ...
    "declaration": true,
    "emitDeclarationOnly": true
  },
  ...
}

With a general .ts file, tsc kicks in and flags the non-implemented imports. And with the two additional compiler options, it also makes sure to ignore duplicate identifiers from other files. Keeps the focus on whether the imports exist, and only on that.

Since this is a structural change that might require review, I'm posting this comment here. If you think this is okay, I already have the repo forked and can open an PR

[matt-hernandez]

@becca-bailey Or you could do it 😆 . The change isn't that crazy. Just one last note, if the solution above works, you also have to remove --noEmit from the compile-ts script

[becca-bailey]

Thanks so much @matt-hernandez ! You saved me a couple hours of digging today to figure all of this out myself. Feel free to open your PR so I can check it out!

[becca-bailey]

Once we have the ts-compile script (or another check script) working as expected, I can handle any updates in CI.

[matt-hernandez]

#2107 was made

It required a bit more housekeeping adjustments than I expected, but the overall crux of the idea is there. I'll be on standby if you need me

[matt-hernandez]

@becca-bailey I may have something! I looked over #2084 which was the issue that first raised the bug of unimplemented types. That issue specifies that a typescript project which uses Victory will only fail to compile if skipLibCheck in the project's tsconfig.json is false.

So I did a little experiment and set tsconfig.json of the Victory repo to this

{
  "compilerOptions": {
    "baseUrl": ".",
    "outDir": "demo/dist",
    "rootDir": ".",
    "module": "es6",
    "target": "es5",
    "jsx": "react",
    "sourceMap": true,
    "moduleResolution": "node",
+    "typeRoots": [
+      "./node_modules/@types"
+    ],
+    "types": ["react", "lodash"],
    "allowSyntheticDefaultImports": true,
    "forceConsistentCasingInFileNames": true,
    "noImplicitReturns": true,
    "noImplicitThis": true,
    "noImplicitAny": true,
    "noUnusedLocals": true,
    "strictNullChecks": true,
    "strict": true,
    "suppressImplicitAnyIndexErrors": true,
    "paths": {
      "@packages/*": ["packages/*"]
    },
-    "skipLibCheck": true
+    "skipLibCheck": false
  },
  "include": [
    "**/src/*",
    "demo/ts/*"
  ],
  "exclude": [
      "node_modules",
      "**/*.spec.ts"
  ]
}

And sure enough, it worked!

This is a MUCH less drastic change than the one in my PR, but since I'm relatively new to Victory as a contributor, I would prefer if you showed this solution to other contributors for feedback. If it looks good and there are no anticipated side effects, than I can update my PR with this change and reverse the original ones. I'm much more in favor of something like this. Much smaller and hopefully more granular

[becca-bailey]

Sounds good, thanks @matt-hernandez! Apologies that it has taken me a bit of extra time to come back to this. I agree that the smaller change is probably better for right now.

[becca-bailey]

Changing the skipLibCheck tsconfig was also recommended by the other contributors I asked. It's up to you whether you want to close your original PR and open a new one with your proposed tsconfig changes, or just update your existing PR to make only these changes. Thanks so much for following up on this!

[matt-hernandez]

Updating my existing PR