Skip to content
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

Merged
merged 30 commits into from
Dec 3, 2021
Merged

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Nov 23, 2021

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 tsconfig files and the addition of standardized templates for each type of tsconfig file.

In the new inheritance structure,

  • tsconfig.x.json files now only exist at the package level, and all inherit from their sibling tsconfig.json,
  • package-level tsconfig.json files all inherit from the repo-level tsconfig.json, and
  • the repo-level tsconfig.json inherits from @sentry/typescript's tsconfig.json.

In other words, the inheritance tree now looks like this:

                                    tsconfig.json (@sentry/typescript)
                                                    |
                                                    |
                                    tsconfig.json (sentry-javascript)
                                    /                                \
                                   /                                  \
                           packages/angular           (same structure for other packages)
                                  |
                                  |
                            tsconfig.json             
                           /      |     \
                          /       |      \
             tsconfig.esm.json    |    tsconfig.test.json
                                  |
                          tsconfig.cjs.json

This new structure allowed for a few other changes:

  • Configuration for tests has been split into a separate tsconfig.test.json. This allows the include values to be specific to either source code or tests, which in turn allows rootDir to be calculated automatically (as the closest common ancestor of all included files).
  • All tsconfig.build.json files have been renamed to tsconfig.cjs.json, for parallel naming structure with existing tsconfig.esm.json files.
  • Both tsconfig.cjs.json and tsconfig.esm.json have been boiled down to only what makes them cjs- or esm-y (their module value and output directory), with all package-level config moved to the package's main tsconfig.json. (It was previously repeated in each file). Note: technically the module: "commonjs" in the cjs files is redundant, since it's implied by the target: "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.
  • Since everything now inherits from the repo-level tsconfig.json, which currently includes node types, all node types elsewhere could be (and have been) removed.

Other notable changes:

  • yarn workspaces (which we already have set up) can handle the cross-package dependencies we were managing through the repo-level paths setting, so that setting has been removed, allowing both its accompanying baseUrl setting and the package-level baseUrl settings necessary to override the top-level baseUrl setting to be removed.
  • declarationMap: false has been removed from the spots that had it, since it hurts nothing to generate declaration maps and this lets us standardize on the true value set in @sentry/typescript.
  • All exclude settings have been removed, since they excluded files which were never in the in include in the first place.
  • All include settings 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 .tsx files.
  • eslint settings have been adjusted to account for the separation of the test config into its own file.
  • package.json scripts have been adjusted to account for the build -> cjs renaming. The old es5 scripts 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 new cjs scripts.
  • rollup config files were adjusted to use tsconfig.esm.json rather than tsconfig.cjs.json (obviating the need to specify the module), and the baseUrl and declarationMap settings 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 diff to verify that they were identical. Here's the script I used:

test_tsconfig.sh
# test_tsconfig.sh

build_dirs=(build dist esm)

function start_clean() {
  yarn clean && rm -rf node_modules && yarn cache clean && yarn install

  for build_dir in "${build_dirs[@]}"; do
    rm -rf packages/*/$build_dir-branch
    rm -rf packages/*/$build_dir-master
  done
}

function remove_commit_hash_from_rollup_config() {
  # remove the commit hash from the contents of the bundle, since those are guaranteed not to match when comparing a
  # branch build to a master build
  for bundlable_package in browser tracing vue; do
    cp packages/$bundlable_package/rollup.config.js packages/$bundlable_package/rollup.config.js.backup

    # macs need an empty string placeholder after the `-i`
    if [[ $(uname) == "Darwin" ]]; then
      sed -i "" s/{commitHash}// packages/$bundlable_package/rollup.config.js
    else
      sed -i s/{commitHash}// packages/$bundlable_package/rollup.config.js
    fi
  done
}

function turn_off_declaration_maps() {
  for package_name in integrations wasm; do
    cp packages/$package_name/tsconfig.json packages/$package_name/tsconfig.json.backup

    sed -i "" s/"package-specific options"/"package-specific options\n    \"declarationMap\": false,"/ packages/$package_name/tsconfig.json
  done
}

function reset_rollup_config() {
  for bundlable_package in browser tracing vue; do
    mv -f packages/$bundlable_package/rollup.config.js.backup packages/$bundlable_package/rollup.config.js
  done
}

function reset_declaration_map_changes() {
  for package_name in integrations wasm; do
    mv -f packages/$package_name/tsconfig.json.backup packages/$package_name/tsconfig.json
  done
}

function move_built_assets() {
  local identifier=$1

  for abs_package_path in $js_repo/packages/*; do
    local package_name=$(basename $abs_package_path)

    for build_dir in "${build_dirs[@]}"; do
      if [[ -d packages/$package_name/$build_dir ]]; then
        mv packages/$package_name/$build_dir/ packages/$package_name/$build_dir-$identifier/
      fi
    done

  done
}

function run_tests() {

  local js_repo=$(pwd)

  git checkout kmclb-unify-tsconfigs
  local branch_base=$(git merge-base master HEAD)

  start_clean

  # prevent the two known sources of difference - the commit hash in the bundles and declaration maps in integrations and wasm
  remove_commit_hash_from_rollup_config
  turn_off_declaration_maps

  yarn build

  # rename all built assets so they're not overwritten by the next build
  move_built_assets "branch"

  reset_rollup_config
  reset_declaration_map_changes

  git checkout $branch_base

  remove_commit_hash_from_rollup_config

  yarn build

  move_built_assets "master"

  # run the diffs
  for abs_package_path in $js_repo/packages/*; do
    local package_name=$(basename $abs_package_path)
    echo "Checking $package_name"

    for build_dir in "${build_dirs[@]}"; do

      if [[ -d packages/$package_name/$build_dir-master ]]; then

        local diff=$(git diff --no-index packages/$package_name/$build_dir-branch packages/$package_name/$build_dir-master)

        if [[ $diff == "" ]]; then
          echo "\t$build_dir diff OK"
        else
          echo "\t$build_dir assets don't match!"
          return
        fi

      fi
    done
  done

  echo "Everything matched!"

  reset_rollup_config

  rm -rf packages/*/dist-branch packages/*/esm-branch packages/*/build-branch
  rm -rf packages/*/dist-master packages/*/esm-master packages/*/build-master

  git checkout kmclb-unify-tsconfigs

}

run_tests

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 -> cjs change happens relatively late in the sequence, so for most commits the build name is still in place (but that's intentional, not an oversight).

@github-actions
Copy link
Contributor

github-actions bot commented Nov 23, 2021

size-limit report

Path Base Size (bfd74df) Current Size Change
@sentry/browser - CDN Bundle (gzipped) 22.45 KB 22.45 KB +0.02% 🔺
@sentry/browser - Webpack 23.29 KB 23.29 KB 0%
@sentry/react - Webpack 23.32 KB 23.32 KB 0%
@sentry/nextjs Client - Webpack 47.98 KB 47.98 KB 0%
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 29.9 KB 29.9 KB +0.01% 🔺

@AbhiPrasad AbhiPrasad self-requested a review November 23, 2021 18:09
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would just 🔥 these comments in all of the tsconfigs, we can leave it in the template

Copy link
Member Author

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.

tsconfig_templates/README.md Outdated Show resolved Hide resolved
"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 ***",
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

packages/angular/tsconfig.cjs.json Show resolved Hide resolved
@lobsterkatie
Copy link
Member Author

Let's cut a beta off this branch before merging to make release works as expected.

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?

@AbhiPrasad
Copy link
Member

Cutting betas still cost us nothing, it'll be good to sanity check everything.

@lobsterkatie
Copy link
Member Author

lobsterkatie commented Nov 29, 2021

Let's cut a beta off this branch before merging to make release works as expected.

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.").

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can ship without the beta, lets get this done!

@lobsterkatie lobsterkatie force-pushed the kmclb-unify-tsconfigs branch from 3d89638 to 797ce20 Compare December 3, 2021 20:28
@lobsterkatie
Copy link
Member Author

Rebased and ran the script again.

image

Going to merge!

@lobsterkatie lobsterkatie merged commit bf36d54 into master Dec 3, 2021
@lobsterkatie lobsterkatie deleted the kmclb-unify-tsconfigs branch December 3, 2021 21:00
lobsterkatie added a commit that referenced this pull request Dec 7, 2021
…#4243)

Follow up to #4178.

This fixes two spots which were missed when converting the build command from `build:es5` to `build:cjs`.
onurtemizkan pushed a commit that referenced this pull request Dec 19, 2021
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 `tsconfig` files and the addition of standardized templates for each type of `tsconfig` file.

In the new inheritance structure,
- `tsconfig.x.json` files now only exist at the package level, and all inherit from their sibling `tsconfig.json`,
- package-level `tsconfig.json` files all inherit from the repo-level `tsconfig.json`, and
- the repo-level `tsconfig.json` inherits from `@sentry/typescript`'s `tsconfig.json`.

In other words, the inheritance tree now looks like this:

```
                                    tsconfig.json (@sentry/typescript)
                                                    |
                                                    |
                                    tsconfig.json (sentry-javascript)
                                    /                                \
                                   /                                  \
                           packages/angular           (same structure for other packages)
                                  |
                                  |
                            tsconfig.json             
                           /      |     \
                          /       |      \
             tsconfig.esm.json    |    tsconfig.test.json
                                  |
                          tsconfig.cjs.json
```

This new structure allowed for a few other changes:
- Configuration for tests has been split into a separate `tsconfig.test.json`. This allows the `include` values to be specific to either source code or tests, which in turn allows `rootDir` to be calculated automatically (as the closest common ancestor of all included files).
- All `tsconfig.build.json` files have been renamed to `tsconfig.cjs.json`, for parallel naming structure with existing `tsconfig.esm.json` files.
- Both `tsconfig.cjs.json` and `tsconfig.esm.json` have been boiled down to only what makes them cjs- or esm-y (their `module` value and output directory), with all package-level config moved to the package's main `tsconfig.json`. (It was previously repeated in each file). Note: technically the `module: "commonjs"` in the cjs files is redundant, since it's implied by the `target: "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.
- Since everything now inherits from the repo-level `tsconfig.json`, which currently includes node types, all node types elsewhere could be (and have been) removed.

Other notable changes:
- `yarn` workspaces (which we already have set up) can handle the cross-package dependencies we were managing through the repo-level `paths` setting, so that setting has been removed, allowing both its accompanying `baseUrl` setting and the package-level `baseUrl` settings necessary to override the top-level `baseUrl` setting to be removed.
- `declarationMap: false` has been removed from the spots that had it, since it hurts nothing to generate declaration maps and this lets us standardize on the `true` value set in `@sentry/typescript`.
- All `exclude` settings have been removed, since they excluded files which were never in the in `include` in the first place.
- All `include` settings 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 `.tsx` files.
- `eslint` settings have been adjusted to account for the separation of the test config into its own file.
- `package.json` scripts have been adjusted to account for the `build` -> `cjs` renaming. The old `es5` scripts 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 new `cjs` scripts.
- rollup config files were adjusted to use `tsconfig.esm.json` rather than `tsconfig.cjs.json` (obviating the need to specify the module), and the `baseUrl` and `declarationMap` settings 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 diff` to verify that they were identical. The script I used can be found in the PR description.
onurtemizkan pushed a commit that referenced this pull request Dec 19, 2021
…#4243)

Follow up to #4178.

This fixes two spots which were missed when converting the build command from `build:es5` to `build:cjs`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants