-
Notifications
You must be signed in to change notification settings - Fork 0
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
update tsconfig file and some tweaks to functions #6
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a good starting point. I've left some thoughts inline, but they are in no way meant to be directives, just suggestions/questions!
tsconfig.json
Outdated
// "emitDecoratorMetadata": true, /* Enables experimental support for emitting type metadata for decorators. */ | ||
}, | ||
"include": ["src/**/*", "./jest-setup.ts"] | ||
"target": "es5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the tsconfig documentation: "Modern browsers support all ES6 features, so ES6 is a good choice." Support does seem pretty good according to https://caniuse.com/?search=es6. I don't think we have (m)any users still on IE 11 at this point? But we should maybe try to check some analytics to validate that assumption.
tsconfig.json
Outdated
}, | ||
"include": ["src/**/*", "./jest-setup.ts"] | ||
"target": "es5", | ||
"module": "esnext", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value is "CommonJS" when target=es5. Do we have a reason to stray from that? If we change target to es6, should we also set this to es6?
tsconfig.json
Outdated
"include": ["src/**/*", "./jest-setup.ts"] | ||
"target": "es5", | ||
"module": "esnext", | ||
"lib": ["dom", "dom.iterable", "esnext"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have sensible defaults based on target
. Should we just rely on those?
tsconfig.json
Outdated
"noImplicitReturns": true, | ||
"noFallthroughCasesInSwitch": true, | ||
"moduleResolution": "node", | ||
"baseUrl": "./", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rootDir=./src
requires that all input files be in ./src
, so I think that means it would make sense for baseUrl=./src
also? And then I think paths
could just be left out?
tsconfig.json
Outdated
"typeRoots": ["./@types", "./node_modules/@types"], | ||
"resolveJsonModule": true, | ||
"allowJs": true, | ||
"checkJs": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for leaving these JS settings as false in the default config and let consumers override them if necessary. I think really this should only be used for incrementally introducing TS to an existing JS codebase, which is a pretty opt-in scenario.
tsconfig.json
Outdated
"esModuleInterop": true, | ||
"typeRoots": ["./@types", "./node_modules/@types"], | ||
"resolveJsonModule": true, | ||
"allowJs": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the JS settings should be in sync. Either both false or both true.
trying out this as a base for a generic
tsconfig
to use in all/most TS projects.