fix(build): Fix temporary sucrase CI jobs (#5021)

lobsterkatie · web-flow · commit 69e62758a42f · 2022-04-29T16:08:17.000-07:00
Until we officially switch over to rullup/sucrase builds, there are extra CI jobs running to test the build changes so far without us having had to switch to them yet. That said, the first attempt at creating such a system had a significant flaw, in that the supposedly-sucrase unit tests were actually running against the normal tsc build output. Further, it was very flaky, but it seems there might occasionally have been some scope bleed between the tsc-built file cache and the sucrase-built one. (It seems unlikely, I know, and I could be imagining it. Regardless, the changes here make it a moot point.)

This fixes those unit tests to run against sucrase builds, by adding a script to build with sucrase in the test job itself (and all other sucrase test jobs). It also moves a prior hack (rebuilding in es5 for node 8 tests) from the main test script into the new hacky script, so it can't get missed when pulling these temporary fixes out of the code when we switch. (That move is why our regular unit tests also run that script, to get the prior hack.) Finally, it makes a temporary modification to our base rollup config when in node 8, to enable the just-in-time rollup builds.

Disclaimer: This is definitely a hack, and is a little ugly, but it's also coming out in a week, so... ¯\\_(ツ)_/¯

Note: Node 8 sucrase tests are an xfail at this point, until more of the new build pipeline is merged.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -105,45 +105,6 @@ jobs:
       # `job_build` can't see `job_install_deps` and what it returned)
       dependency_cache_key: ${{ needs.job_install_deps.outputs.dependency_cache_key }}
 
-  # This isn't a full `yarn build` using sucrase - it's just the cache from the normal build, with `build/cjs` and
-  # `build/esm` overwritten by sucrase. This way we don't need to worry about all of the other random stuff which
-  # packages build, because it will already be there.
-  job_build_with_sucrase:
-    name: Sucrase Build
-    needs: [job_install_deps, job_build]
-    runs-on: ubuntu-latest
-    timeout-minutes: 20
-    steps:
-      - name: Check out current commit (${{ env.HEAD_COMMIT }})
-        uses: actions/checkout@v2
-        with:
-          ref: ${{ env.HEAD_COMMIT }}
-      - name: Set up Node
-        uses: actions/setup-node@v1
-      - name: Check dependency cache
-        uses: actions/cache@v2
-        with:
-          path: ${{ env.CACHED_DEPENDENCY_PATHS }}
-          key: ${{ needs.job_install_deps.outputs.dependency_cache_key }}
-      - name: Check tsc build cache
-        uses: actions/cache@v2
-        with:
-          path: ${{ env.CACHED_BUILD_PATHS }}
-          key: ${{ env.BUILD_CACHE_KEY }}
-      - name: Check sucrase build cache
-        uses: actions/cache@v2
-        id: cache_built_sucrase_packages
-        with:
-          path: ${{ env.CACHED_BUILD_PATHS }}
-          key: ${{ env.BUILD_CACHE_KEY }}-sucrase
-      - name: Build packages with sucrase
-        if: steps.cache_built_sucrase_packages.outputs.cache-hit == ''
-        run: 'yarn build:rollup'
-    outputs:
-      # this needs to be passed on, because the `needs` context only looks at direct ancestors (so steps which depend on
-      # `job_build` can't see `job_install_deps` and what it returned)
-      dependency_cache_key: ${{ needs.job_install_deps.outputs.dependency_cache_key }}
-
   job_size_check:
     name: Size Check
     needs: job_build
@@ -295,6 +256,8 @@ jobs:
           NODE_VERSION: ${{ matrix.node }}
         run: |
           [[ $NODE_VERSION == 8 ]] && yarn add --dev --ignore-engines --ignore-scripts --ignore-workspace-root-check ts-node@8.x
+          # TODO remove this when we switch to sucrase
+          yarn ts-node scripts/sucrase-test-hack.ts
           yarn test-ci
       - name: Compute test coverage
         uses: codecov/codecov-action@v1
@@ -534,7 +497,7 @@ jobs:
 
   job_unit_test_sucrase:
     name: Sucrase Test (Node ${{ matrix.node }})
-    needs: job_build_with_sucrase
+    needs: job_build
     continue-on-error: true
     timeout-minutes: 30
     runs-on: ubuntu-latest
@@ -554,24 +517,26 @@ jobs:
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_DEPENDENCY_PATHS }}
-          key: ${{ needs.job_build_with_sucrase.outputs.dependency_cache_key }}
+          key: ${{ needs.job_build.outputs.dependency_cache_key }}
       - name: Check build cache
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_BUILD_PATHS }}
-          key: ${{ env.BUILD_CACHE_KEY }}-sucrase
+          key: ${{ env.BUILD_CACHE_KEY }}
       - name: Run tests
         env:
           NODE_VERSION: ${{ matrix.node }}
+          SUCRASE: true
         run: |
-          [[ $NODE_VERSION == 8 ]] && yarn add --dev --ignore-engines --ignore-scripts --ignore-workspace-root-check ts-node@8.x
+          [[ $NODE_VERSION == 8 ]] && yarn add --dev --ignore-engines --ignore-scripts --ignore-workspace-root-check ts-node@8.x rollup@2.2.0 @rollup/plugin-node-resolve@10.x rollup-plugin-terser@5.0.0 rollup-plugin-typescript2@0.30.0
+          yarn ts-node scripts/sucrase-test-hack.ts
           yarn test-ci
       - name: Compute test coverage
         uses: codecov/codecov-action@v1
 
   job_nextjs_integration_test_sucrase:
     name: Sucrase Test @sentry/nextjs on (Node ${{ matrix.node }})
-    needs: job_build_with_sucrase
+    needs: job_build
     continue-on-error: true
     timeout-minutes: 30
     runs-on: ubuntu-latest
@@ -591,24 +556,25 @@ jobs:
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_DEPENDENCY_PATHS }}
-          key: ${{ needs.job_build_with_sucrase.outputs.dependency_cache_key }}
+          key: ${{ needs.job_build.outputs.dependency_cache_key }}
       - name: Check build cache
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_BUILD_PATHS }}
-          key: ${{ env.BUILD_CACHE_KEY }}-sucrase
+          key: ${{ env.BUILD_CACHE_KEY }}
       - name: Run tests
         env:
           NODE_VERSION: ${{ matrix.node }}
         run: |
+          yarn ts-node scripts/sucrase-test-hack.ts
           cd packages/nextjs
           yarn test:integration
 
   # Ember tests are separate from the rest because they are the slowest part of the test suite, and making them a
   # separate job allows them to run in parallel with the other tests.
   job_ember_tests_sucrase:
     name: Sucrase Test @sentry/ember
-    needs: job_build_with_sucrase
+    needs: job_build
     continue-on-error: true
     timeout-minutes: 30
     runs-on: ubuntu-latest
@@ -634,20 +600,22 @@ jobs:
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_DEPENDENCY_PATHS }}
-          key: ${{ needs.job_build_with_sucrase.outputs.dependency_cache_key }}
+          key: ${{ needs.job_build.outputs.dependency_cache_key }}
       - name: Check build cache
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_BUILD_PATHS }}
-          key: ${{ env.BUILD_CACHE_KEY }}-sucrase
+          key: ${{ env.BUILD_CACHE_KEY }}
       - name: Run Ember tests
-        run: yarn test --scope=@sentry/ember
+        run: |
+          yarn ts-node scripts/sucrase-test-hack.ts
+          yarn test --scope=@sentry/ember
       - name: Compute test coverage
         uses: codecov/codecov-action@v1
 
   job_browser_playwright_tests_sucrase:
     name: Sucrase Playwright - ${{ (matrix.tracing_only && 'Browser + Tracing') || 'Browser' }} (${{ matrix.bundle }})
-    needs: job_build_with_sucrase
+    needs: job_build
     runs-on: ubuntu-latest
     strategy:
       matrix:
@@ -677,24 +645,25 @@ jobs:
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_DEPENDENCY_PATHS }}
-          key: ${{ needs.job_build_with_sucrase.outputs.dependency_cache_key }}
+          key: ${{ needs.job_build.outputs.dependency_cache_key }}
       - name: Check build cache
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_BUILD_PATHS }}
-          key: ${{ env.BUILD_CACHE_KEY }}-sucrase
+          key: ${{ env.BUILD_CACHE_KEY }}
       - name: Run Playwright tests
         env:
           PW_BUNDLE: ${{ matrix.bundle }}
           PW_TRACING_ONLY: ${{ matrix.tracing_only }}
         run: |
+          yarn ts-node scripts/sucrase-test-hack.ts
           cd packages/integration-tests
           yarn run playwright install-deps webkit
           yarn test:ci
 
   job_browser_integration_tests_sucrase:
     name: Sucrase Old Browser Integration Tests (${{ matrix.browser }})
-    needs: job_build_with_sucrase
+    needs: job_build
     runs-on: ubuntu-latest
     timeout-minutes: 10
     continue-on-error: true
@@ -715,23 +684,24 @@ jobs:
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_DEPENDENCY_PATHS }}
-          key: ${{ needs.job_build_with_sucrase.outputs.dependency_cache_key }}
+          key: ${{ needs.job_build.outputs.dependency_cache_key }}
       - name: Check build cache
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_BUILD_PATHS }}
-          key: ${{ env.BUILD_CACHE_KEY }}-sucrase
+          key: ${{ env.BUILD_CACHE_KEY }}
       - name: Run integration tests
         env:
           KARMA_BROWSER: ${{ matrix.browser }}
         run: |
+          yarn ts-node scripts/sucrase-test-hack.ts
           cd packages/browser
           [[ $KARMA_BROWSER == WebkitHeadless ]] && yarn run playwright install-deps webkit
           yarn test:integration
 
   job_browser_build_tests_sucrase:
     name: Sucrase Browser Build Tests
-    needs: job_build_with_sucrase
+    needs: job_build
     runs-on: ubuntu-latest
     timeout-minutes: 5
     continue-on-error: true
@@ -748,14 +718,15 @@ jobs:
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_DEPENDENCY_PATHS }}
-          key: ${{ needs.job_build_with_sucrase.outputs.dependency_cache_key }}
+          key: ${{ needs.job_build.outputs.dependency_cache_key }}
       - name: Check build cache
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_BUILD_PATHS }}
-          key: ${{ env.BUILD_CACHE_KEY }}-sucrase
+          key: ${{ env.BUILD_CACHE_KEY }}
       - name: Run browser build tests
         run: |
+          yarn ts-node scripts/sucrase-test-hack.ts
           cd packages/browser
           yarn test:package
       - name: Run utils build tests
@@ -765,7 +736,7 @@ jobs:
 
   job_node_integration_tests_sucrase:
     name: Sucrase Node SDK Integration Tests (${{ matrix.node }})
-    needs: job_build_with_sucrase
+    needs: job_build
     runs-on: ubuntu-latest
     timeout-minutes: 10
     continue-on-error: true
@@ -785,15 +756,16 @@ jobs:
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_DEPENDENCY_PATHS }}
-          key: ${{ needs.job_build_with_sucrase.outputs.dependency_cache_key }}
+          key: ${{ needs.job_build.outputs.dependency_cache_key }}
       - name: Check build cache
         uses: actions/cache@v2
         with:
           path: ${{ env.CACHED_BUILD_PATHS }}
-          key: ${{ env.BUILD_CACHE_KEY }}-sucrase
+          key: ${{ env.BUILD_CACHE_KEY }}
       - name: Run integration tests
         env:
           NODE_VERSION: ${{ matrix.node }}
         run: |
+          yarn ts-node scripts/sucrase-test-hack.ts
           cd packages/node-integration-tests
           yarn test
diff --git a/rollup/npmHelpers.js b/rollup/npmHelpers.js
@@ -22,7 +22,8 @@ export function makeBaseNPMConfig(options = {}) {
   const nodeResolvePlugin = makeNodeResolvePlugin();
   const sucrasePlugin = makeSucrasePlugin();
 
-  return {
+  // return {
+  const config = {
     input: entrypoints,
 
     output: {
@@ -32,10 +33,10 @@ export function makeBaseNPMConfig(options = {}) {
       sourcemap: true,
 
       // output individual files rather than one big bundle
-      preserveModules: true,
+      // preserveModules: true,
 
       // any wrappers or helper functions generated by rollup can use ES6 features
-      generatedCode: 'es2015',
+      // generatedCode: 'es2015',
 
       // don't add `"use strict"` to the top of cjs files
       strict: false,
@@ -78,6 +79,17 @@ export function makeBaseNPMConfig(options = {}) {
     // treeshake: 'smallest',
     treeshake: false,
   };
+
+  // temporary hack for testing sucrase bundles before we switch over
+  if (!process.version.startsWith('v8')) {
+    console.log('Doing normal preserveModules');
+    config.output.preserveModules = true;
+  } else {
+    console.log('Doing node 8 preserveModules');
+    config.preserveModules = true;
+  }
+
+  return config;
 }
 
 export function makeNPMConfigVariants(baseConfig) {
diff --git a/scripts/sucrase-test-hack.ts b/scripts/sucrase-test-hack.ts
@@ -0,0 +1,49 @@
+// A temporary hack to use sucrase versions of packages for testing in CI.
+
+import * as childProcess from 'child_process';
+import * as fs from 'fs';
+import * as path from 'path';
+
+function run(cmd: string, options?: childProcess.ExecSyncOptions): unknown {
+  return childProcess.execSync(cmd, { stdio: 'inherit', ...options });
+}
+
+const ignorePackages = process.version.startsWith('v8')
+  ? [
+      '@sentry/ember',
+      '@sentry-internal/eslint-plugin-sdk',
+      '@sentry/react',
+      '@sentry/wasm',
+      '@sentry/gatsby',
+      '@sentry/serverless',
+      '@sentry/nextjs',
+      '@sentry/angular',
+    ]
+  : ['@sentry/serverless'];
+
+// clear current builds and rebuild with rollup/sucrase (this way, all of the extra, random stuff which gets built in
+// the main build job remains, and only the part affected by this project gets overwritten)
+if (process.env.SUCRASE) {
+  // just to be super sure
+  fs.readdirSync(path.join(process.cwd(), 'packages')).forEach(dir => {
+    if (fs.existsSync(path.join(process.cwd(), 'packages', dir, 'build', 'npm'))) {
+      run(`rm -rf packages/${dir}/build/npm/cjs`);
+      run(`rm -rf packages/${dir}/build/npm/esm`);
+    } else if (fs.existsSync(path.join(process.cwd(), 'packages', dir, 'build', 'cjs'))) {
+      run(`rm -rf packages/${dir}/build/cjs`);
+      run(`rm -rf packages/${dir}/build/esm`);
+    }
+  });
+
+  // rebuild the packages we're going to test with rollup/sucrase
+  run(`yarn build:rollup ${ignorePackages.map(dep => `--ignore="${dep}"`).join(' ')}`);
+}
+// if we're in tsc-land, rebuild using es5 - temporary until switch to sucrase
+else {
+  const baseTSConfigPath = 'packages/typescript/tsconfig.json';
+  fs.writeFileSync(
+    baseTSConfigPath,
+    String(fs.readFileSync(baseTSConfigPath)).replace('"target": "es6"', '"target": "es5"'),
+  );
+  run(`yarn build:dev ${ignorePackages.map(dep => `--ignore="${dep}"`).join(' ')}`);
+}
diff --git a/scripts/test.ts b/scripts/test.ts
@@ -1,5 +1,4 @@
 import { spawnSync } from 'child_process';
-import * as fs from 'fs';
 import { join } from 'path';
 
 function run(cmd: string, cwd: string = '') {
@@ -48,12 +47,6 @@ if (nodeMajorVersion <= 10) {
     // against a single version of node, but in the short run, this at least allows us to not be blocked by the
     // failures.)
     run('rm -rf packages/tracing/test/browser');
-
-    // TODO Pull this out once we switch to sucrase builds
-    // Recompile as es5, so as not to have to fix a compatibility problem that will soon be moot
-    const baseTSConfig = 'packages/typescript/tsconfig.json';
-    fs.writeFileSync(baseTSConfig, String(fs.readFileSync(baseTSConfig)).replace('"target": "es6"', '"target": "es5"'));
-    run(`yarn build:dev ${ignorePackages.map(dep => `--ignore="${dep}"`).join(' ')}`);
   }
   // Node 10
   else {
