fix: spurious typescript errors #1550
Conversation
| "noImplicitAny": true, | ||
| "noImplicitThis": true, | ||
| "moduleResolution": "node", | ||
| "module": "commonjs", | ||
| "outDir": "build", |
There was a problem hiding this comment.
This was the reason why we were seeing so many errors in the vscode intellisense - we needed a default output directory so ts wasn't trying to override our javascript files with themselves
tsconfig.json
Outdated
| "skipLibCheck": true | ||
| "skipLibCheck": true, | ||
| "downlevelIteration": true, | ||
| "target": "es5" |
There was a problem hiding this comment.
2017 is way too modern a target, as Android 5 was released in 2014. I've added downlevelIteration to support this (this allows support for compiling Iterators to this target)
There was a problem hiding this comment.
I think we specify the target in the webpack, no?
es5 seems scary, and unnecessary for Electron.
There was a problem hiding this comment.
Mm, on second look we do have a BABEL_LOADER which uses @babel/preset-env which ascends up to the system root, looking for the browserlist. So it's there, but not very explicit.
What if I were to set the target here to "esnext" instead and add a comment pointing to the babel config?
There was a problem hiding this comment.
Actually there's also an es5 target here: https://github.com/Jigsaw-Code/outline-client/blob/8a3f54cb96d1f9afa231824cb9dd761c42bc26bc/src/www/webpack_cordova.mjs#L36-L37
There was a problem hiding this comment.
BUT our electron build doesn't use preset_env: https://github.com/Jigsaw-Code/outline-client/blob/8a3f54cb96d1f9afa231824cb9dd761c42bc26bc/src/www/webpack_electron.mjs#L24-L41
There was a problem hiding this comment.
Ugh, these things should live in src/cordova and src/electron respectively. I'll remove the target change for now and we can clean this up later.
| // We don't need TypeScript to check javascript files. | ||
| "include": ["./src/**/*.ts"], | ||
| "exclude": ["node_modules", "www", "./src/**/*.js", "./src/**/*.mjs", "./www/**/*", "./build/**/*"] | ||
| "exclude": ["*.cjs", "*.js", "*.mjs"], |
There was a problem hiding this comment.
Shouldn't we exclude node_modules?
There was a problem hiding this comment.
we have an include for src, which scopes all our compilation to src - node_modules is outside of src:
https://www.typescriptlang.org/tsconfig#include
https://www.typescriptlang.org/tsconfig#exclude
"Exclude: Specifies an array of filenames or patterns that should be skipped when resolving include."
tsconfig.json
Outdated
| "skipLibCheck": true | ||
| "skipLibCheck": true, | ||
| "downlevelIteration": true, | ||
| "target": "es5" |
There was a problem hiding this comment.
I think we specify the target in the webpack, no?
es5 seems scary, and unnecessary for Electron.
| "skipLibCheck": true | ||
| "rootDir": "./", | ||
| "skipLibCheck": true, | ||
| "target": "es2017" |
There was a problem hiding this comment.
Do we need to specify a target? Will this affect the build or just VSCode?
If it doesn't affect the build, then esnext seems more appropriate.
There was a problem hiding this comment.
So it seems the es2017 target here is specifically for electron, as we're not using babel in the electron webpack. I'm afraid if I remove or change it, it may break electron. This is way too ambiguous - the needed JS build target and build parameters for electron should live directly in src/electron. I started working on this a while back (#1459) so I think that's the next step here
daniellacosse
changed the title
Yes, sorry I sorted the JSON keys alphabetically |
No description provided.