[Discussion] Enable stricter TypeScript checks

[nx10]

Is your feature request related to a problem? Please describe.

There are currently no additional TypeScript checks enabled in .tsconfig (/~https://github.com/REditorSupport/vscode-R/blob/master/tsconfig.json).

Using strict-mode in TypeScript is recommended: https://www.typescriptlang.org/tsconfig#strict

Opting out on a per-case basis via // @ts-ignore or foo as FooType should be preferred to indicate to other developers that a statement is not an error but a conscious decision.

Describe the solution you'd like

The VSCode Python extension (see) uses the following settings:

    "compilerOptions": {
        "strict": true,
        "noImplicitAny": true,
        "noImplicitThis": true,
        "noUnusedLocals": true,
        "noUnusedParameters": true,
        "noFallthroughCasesInSwitch": true,
    },

Enabling the options below shows quite a number of errors:

> yarn run compile
Found 302 errors in 32 files.

Errors  Files
    16  src/completions.ts:35
    16  src/extension.ts:35
     2  src/helpViewer/cran.ts:83
     2  src/helpViewer/helpProvider.ts:301
     6  src/helpViewer/index.ts:175
     3  src/helpViewer/packages.ts:85
    11  src/helpViewer/panel.ts:72
    15  src/helpViewer/treeView.ts:68
     6  src/languageService.ts:42
     2  src/lineCache.ts:30
     3  src/lintrConfig.ts:12
     5  src/liveShare/index.ts:21
     2  src/liveShare/shareCommands.ts:80
    11  src/liveShare/shareSession.ts:68
    17  src/liveShare/shareTree.ts:14
     5  src/plotViewer/index.ts:289
     3  src/preview.ts:32
     1  src/rGitignore.ts:37
     7  src/rmarkdown/draft.ts:49
    32  src/rmarkdown/index.ts:63
    13  src/rmarkdown/knit.ts:12
    11  src/rmarkdown/manager.ts:32
    23  src/rmarkdown/preview.ts:20
     5  src/rstudioapi.ts:119
    26  src/rTerminal.ts:20
    12  src/selection.ts:8
    14  src/session.ts:185
     1  src/tasks.ts:104
     3  src/test/suite/index.ts:5
     2  src/test/suite/syntax.test.ts:56
    20  src/util.ts:25
     7  src/workspaceViewer.ts:39

(For reference, if only "strict" is enabled it shows Found 295 errors in 31 files.)

PR

If there is agreement on enabling strict mode I volunteer to work on a PR that fixes the existing errors. (I don't want to start working on it if its not desired)

[ManuelHentschel]

I 100% agree and will be happy to help!

Regarding the specific compiler options, I would maybe reconsider "noUnusedLocals": true and "noUnusedParameters": true. Unused variables/parameters don't really create any problems (afaik), and having these flagged as errors might be a bit annoying during development.

[nx10]

I 100% agree and will be happy to help!

Fantastic!

Regarding the specific compiler options, I would maybe reconsider "noUnusedLocals": true and "noUnusedParameters": true. Unused variables/parameters don't really create any problems (afaik), and having these flagged as errors might be a bit annoying during development.

Good point. I agree, this should be linted as a warning not cause an error. If I am not mistaken it also already is via eslint no-unused-vars

[renkun-ken]

Thanks for raising this issue! I totally agree.

[renkun-ken]

@ElianHugh Would you like to test if live share functionality works?

I tried live share, but it does not seem to work as usual as I cannot make guest attach to a new session and have shared plot and View, help tabs. It does not work with the current release either. Not sure if I'm missing something?

Other than live share, everything seems to work nicely.

[nx10]

Thanks for testing. This is likely a bug, I will investigate.

[ElianHugh]

@ElianHugh Would you like to test if live share functionality works?

I tried live share, but it does not seem to work as usual as I cannot make guest attach to a new session and have shared plot and View, help tabs. It does not work with the current release either. Not sure if I'm missing something?

Other than live share, everything seems to work nicely.

Sounds like something wrong with the liveshare API, will investigate. Thanks for the heads up!

Edit: definitely not an issue with the PR. issue is stemming from the shareService API failing for some reason

[ElianHugh]

I've bumped the liveshare thread here, but given this is happening independent of the PR, this shouldn't be a blocker for merging :)