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
ref(dev): Unify typescript config files #4178
Conversation
size-limit report
|
Let's cut a beta off this branch before merging to make release works as expected.
| "include": ["test/**/*"], | ||
|
|
||
| "compilerOptions": { | ||
| // should include all types from `./tsconfig.json` plus types for all test frameworks used |
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.
Would just
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.
Unlike the other comments, which I removed, these I think we should keep, because if you ever were to make changes, you'd need to know this fact.
| "build:dev:watch": "run-s build:watch", | ||
| "build:es5:watch": "tsc -p tsconfig.build.json -w --preserveWatchOutput", | ||
| "build:es5:watch": "yarn build:cjs:watch # *** backwards compatibility - remove in v7 ***", |
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 don't think there is any place that uses them directly, let's just remove.
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.
Could do, but given that there's no harm in leaving it, any reason not to err on the side of caution?
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 guess if we are going back to standardize all the package.json eventually, we can get away with leaving it here for now.
I added my testing script in the PR description. If that passes (which it does for me, but feel free to check it for yourself), I don't think we need a beta branch, do we, given that we'll be releasing identical files before and after this PR? |
|
Cutting betas still cost us nothing, it'll be good to sanity check everything. |
¯\(ツ)/¯ Sure, we can if you want, I just don't get what that lets us check that isn't already checked (i.e., I don't actually know what I'd do with a beta if I had one in terms of extra testing, so that I can say, "I made a beta and ______, and looks like we're good."). |
lobsterkatie
mentioned this pull request



This PR unifies and standardizes our typescript config files, with the goal of making them easier to reason about and easier to maintain.
The two key changes are the simplification of the inheritance between various
tsconfigfiles and the addition of standardized templates for each type oftsconfigfile.In the new inheritance structure,
tsconfig.x.jsonfiles now only exist at the package level, and all inherit from their siblingtsconfig.json,tsconfig.jsonfiles all inherit from the repo-leveltsconfig.json, andtsconfig.jsoninherits from@sentry/typescript'stsconfig.json.In other words, the inheritance tree now looks like this:
This new structure allowed for a few other changes:
tsconfig.test.json. This allows theincludevalues to be specific to either source code or tests, which in turn allowsrootDirto be calculated automatically (as the closest common ancestor of all included files).tsconfig.build.jsonfiles have been renamed totsconfig.cjs.json, for parallel naming structure with existingtsconfig.esm.jsonfiles.tsconfig.cjs.jsonandtsconfig.esm.jsonhave been boiled down to only what makes them cjs- or esm-y (theirmodulevalue and output directory), with all package-level config moved to the package's maintsconfig.json. (It was previously repeated in each file). Note: technically themodule: "commonjs"in the cjs files is redundant, since it's implied by thetarget: "es5"in@sentry/typescript's tsconfig, but I decided to include it, both for parallel structure with the esm files and because, as mentioned, it's the essence of being cjs.tsconfig.json, which currently includes node types, all node types elsewhere could be (and have been) removed.Other notable changes:
yarnworkspaces (which we already have set up) can handle the cross-package dependencies we were managing through the repo-levelpathssetting, so that setting has been removed, allowing both its accompanyingbaseUrlsetting and the package-levelbaseUrlsettings necessary to override the top-levelbaseUrlsetting to be removed.declarationMap: falsehas been removed from the spots that had it, since it hurts nothing to generate declaration maps and this lets us standardize on thetruevalue set in@sentry/typescript.excludesettings have been removed, since they excluded files which were never in the inincludein the first place.includesettings have been simplified to just include everything in the relevant directory, to take advantage of typescript's default of including all.js,.jsx,.ts, and.tsxfiles.eslintsettings have been adjusted to account for the separation of the test config into its own file.package.jsonscripts have been adjusted to account for thebuild->cjsrenaming. The oldes5scripts have been retained, for backwards compatibility (I don't know of any place which uses them directly, but just in case), with a note to remove them in v7, and they now point to the newcjsscripts.tsconfig.esm.jsonrather thantsconfig.cjs.json(obviating the need to specify the module), and thebaseUrlanddeclarationMapsettings removed from the main tsconfigs were ported to the rollup config (where they're actually necessary).I validated these changes by building bundles, cjs, and esm files both under master and this branch, and then comparing the outputs using
git diffto verify that they were identical. Here's the script I used:test_tsconfig.sh
Note to reviewers: There are a lot of individual, small changes here, but I've tried to separate them out so that each commit only covers one idea, across packages. I'd highly recommend reviewing it commit by commit rather than as a 110-file whole. If you do this, note that the
build->cjschange happens relatively late in the sequence, so for most commits thebuildname is still in place (but that's intentional, not an oversight).The text was updated successfully, but these errors were encountered: