Inherit core-tracing tsconfig.json from root #5320
Conversation
xirzec
added
the
Client
There was a problem hiding this comment.
nice!
one nitpick: comments in JSON cause a lot of editors to turn extremely red because they aren't part of the true JSON spec. Is it possible for us to either configure the repo with workspace settings that will ignore that, or alternatively just remove the comments from the JSON? Since tsconfig/rush/etc. are all schematized we should have good enough IntelliSense to not need every option documented inline when we come back to change these files.
tsconfig.json
Outdated
| "target": "ES2015", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */ | ||
| "module": "commonjs", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */ | ||
| "target": "ES2015", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */ | ||
| "module": "commonjs", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */ | ||
| "lib": ["es2017"], /* Specify library files to be included in the compilation. */ |
There was a problem hiding this comment.
I think our guidelines say not to include anything in lib because it will require downstream users to also include it. The eslint plugin at least checks that it is present and set to []
| @@ -0,0 +1,4 @@ | |||
| { | |||
There was a problem hiding this comment.
Do we need to have both this and tsconfig.json next to each other? What's the behavior of just folding tsconfig.json and this together? Do we even need a tsconfig.json in the repo root or could we stash the base config away in /common?
There was a problem hiding this comment.
The problem is that we have CI checks that do npm install on the root (I think for the management libs) and they use gulp... which breaks if we don't use commonjs as the target because ts-gulp doesn't support this right. It always needs a tsconfig.json in the root.
Maybe this is something we can take up with eng sys at some point.
Yeah I don't like JSON+Comments either. I believe @bterlson said he was fine with removing them. |
|
@willmtemple can you take another look? Thanks 👍 |
|
Yep feel free to remove any and all syntactic extensions to JSON. It's just and proper to do so. |
| @@ -1,6 +1,8 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Licensed under the MIT License. | |||
|
|
|||
| /// <reference lib="dom"/> | |||
There was a problem hiding this comment.
Avoid this if at all possible. /// reference directives have the effect of opting our users in to that complete set of types as if they had specified the lib entry themselves. Sometimes, this is what you want. For us, more often we just want TypeScript not to complain, and don't want to augment our users' types any more than necessary for correctness reasons.
Better to declare stub interfaces up front until we get placeholder types. Could do something like this in a d.ts file:
interface Window {};
declare var self: Window & typeof globalThis;There was a problem hiding this comment.
Can you update the guidance?
https://azure.github.io/azure-sdk/typescript_design.html#ts-config-lib
DO NOT use the compilerOptions.lib field. Built in typescript libraries (for example, esnext.asynciterable) should be included via reference directives.
The above also links to the following PR which introduced a /// reference:
https://github.com/Azure/azure-cosmos-js/pull/161/files
There was a problem hiding this comment.
Lib reference removed in latest commit, @bterlson please take another look
Per #5276 , let's start migrating to common tsconfig.json settings rather than having a different set of checks enabled for each package.