Dm 5 tsconfig #17
Dm 5 tsconfig #17
Conversation
|
SonarCloud Quality Gate failed.
|
|
I completely agree and if that's the only reason this is failing the Quality Gate then it's a good thing that's not a required status check–we'll exercise our code review discretion and ignore the duplication as it's a pre-existing issue, this PR does not represent a regression, and this PR improves the overall quality of the target branch by removing tsconfig duplication. |
|
These changes standardize the front-import paths so "front-end" always stands in for "front-end/typescript." This means no 1,000 changes to front-end file import paths. The two testing |
| @@ -1,5 +1,5 @@ | |||
| const path = require('path'); | |||
| const compilerOptions = require('./tsconfig.json').compilerOptions; | |||
| const compilerOptions = require('../../tsconfig.json').compilerOptions; | |||
|
|
|||
| process.chdir(__dirname); | |||
|
|
|||
There was a problem hiding this comment.
I'm not sure if the block starting line 11 should also use the root tsconfig--struggled with the documentation: https://www.npmjs.com/package/ts-node#:~:text=ts-node%20and-,register%20the%20loader,-for%20future%20requires
There was a problem hiding this comment.
I wouldn't worry too much about it yet since the existing mocha suite runs just one example test. I'd split the work of improving or replacing the mocha harness to a new PR, and since we're reading a property out of a json file and that property is from the parent this looks right to me.
There was a problem hiding this comment.
To be absolutely correct we'd want to read both properties out of both json files and then merge together the compilerOptions sub-keys the way typescript does...but this is working now (it's in a better state than in main) and we're only missing the override of target and noEmit keys from the overridden child compilerOptions.
There was a problem hiding this comment.
Maybe add a tech debt card about merging the compilerOptions keys from the two files?
There was a problem hiding this comment.
Will combine compilerOptions as part of this PR. Came across some conversation on registering ts-node, so I'm more confident line 11 is right as is.
This PR:
A few things I'd love feedback on:
npm run front-end:watch,npm run back-end:watch,npm run front-test,npm run back-end:test,npm run migrations:latestto confirm the changes hadn't caused compilation errors. I think the front-/back-end scripts would have covered my changes to thesharedtsconfig, but I'm not sure about thescriptsonetsconfigs, so we left them in for now (late-breaking thought--I think the issue is related to using mocha (the paths are imported from thetsconfiginto'index.js), so once we've switched to jest, it won't be an issue)