From 010ce8c20f6ce129bc800eb248b1ba66ac99a533 Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 11:25:16 -0500 Subject: [PATCH 01/16] Remove tests from build output --- tsconfig.build.json | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tsconfig.build.json b/tsconfig.build.json index 2a2bd2c3c6d..561345472aa 100644 --- a/tsconfig.build.json +++ b/tsconfig.build.json @@ -1,7 +1,12 @@ { "extends": "./tsconfig.json", - // NOTE: We exclude Storybook stories — in part because we don't want their type definitions - // included in our build, but also because _something_ in Storybook mucks with the type definitions - // of Primer components. See also https://github.com/primer/react/issues/1163. - "exclude": ["**/*.stories.tsx", "script/**/*.ts"] + // NOTE: We exclude Storybook stories and test utilities which import + // Storybook and its dependencies, because: + // a) we don't want Storybook types in our build output, and + // b) we don't want transitive dependencies, like @emotion/core, to have + // their React interface augmentations in our build output. + // See also: + // - https://github.com/primer/react/issues/1163 + // - https://github.com/primer/react/issues/1849 + "exclude": ["**/*.stories.tsx", "script/**/*.ts", "src/__tests__/", "src/utils/test-*.tsx", "src/utils/testing.tsx"] } From 96c6ce9c8d5771ca139e2f415553509507f6f722 Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 11:35:02 -0500 Subject: [PATCH 02/16] Add test-types workflow --- .github/workflows/test_types.yml | 37 ++++++++++++++++++++++++++++++++ test-types/App.tsx | 5 +++++ test-types/package.json | 5 +++++ test-types/tsconfig.json | 17 +++++++++++++++ 4 files changed, 64 insertions(+) create mode 100644 .github/workflows/test_types.yml create mode 100644 test-types/App.tsx create mode 100644 test-types/package.json create mode 100644 test-types/tsconfig.json diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml new file mode 100644 index 00000000000..32e24607c9b --- /dev/null +++ b/.github/workflows/test_types.yml @@ -0,0 +1,37 @@ +name: Test types +on: + push: {branches: main} + pull_request: {branches: main} + workflow_dispatch: + +jobs: + test-types: + name: Test types + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@v2 + + - name: Set up Node.js + uses: actions/setup-node@v2 + with: + node-version: 16 + cache: npm + + - name: Install dependencies + run: npm ci + + - name: Build + run: npm run build + + - name: Link + run: npm link + + - name: Install only production dependencies + run: npm ci --production + + - name: Link and test + working-directory: test-types + run: | + npm link @primer/react + npm tsc --noEmit \ No newline at end of file diff --git a/test-types/App.tsx b/test-types/App.tsx new file mode 100644 index 00000000000..5eadc1651da --- /dev/null +++ b/test-types/App.tsx @@ -0,0 +1,5 @@ +import {Box} from '@primer/react' + +export default function App() { + return +} \ No newline at end of file diff --git a/test-types/package.json b/test-types/package.json new file mode 100644 index 00000000000..2a2d7fa13da --- /dev/null +++ b/test-types/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "@primer/react": "*" + } +} \ No newline at end of file diff --git a/test-types/tsconfig.json b/test-types/tsconfig.json new file mode 100644 index 00000000000..276f18a6351 --- /dev/null +++ b/test-types/tsconfig.json @@ -0,0 +1,17 @@ +{ + "compilerOptions": { + "skipLibCheck": false, // IMPORTANT: Validates our type outputs + "target": "esnext", + "module": "commonjs", + "allowJs": true, + "checkJs": false, + "jsx": "preserve", + "declaration": true, + "noEmit": true, + "strict": true, + "moduleResolution": "node", + "esModuleInterop": true, + "forceConsistentCasingInFileNames": true, + }, + "include": ["./*.tsx"] +} From 7a11a1895af1804d0645c0fc211dcc0ca0a5546f Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 11:38:31 -0500 Subject: [PATCH 03/16] Add ignore-scripts flag --- .github/workflows/test_types.yml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index 32e24607c9b..8d7eb25d857 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -16,10 +16,9 @@ jobs: uses: actions/setup-node@v2 with: node-version: 16 - cache: npm - name: Install dependencies - run: npm ci + run: npm ci --ignore-scripts - name: Build run: npm run build @@ -28,10 +27,10 @@ jobs: run: npm link - name: Install only production dependencies - run: npm ci --production + run: npm ci --production --ignore-scripts - name: Link and test working-directory: test-types run: | npm link @primer/react - npm tsc --noEmit \ No newline at end of file + npm tsc --noEmit From 7537a531e2533cf20ae8bd626b43529b35a537bd Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 11:43:23 -0500 Subject: [PATCH 04/16] Add ignore-scripts to link --- .github/workflows/test_types.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index 8d7eb25d857..2b8fd54f19f 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -23,14 +23,14 @@ jobs: - name: Build run: npm run build - - name: Link - run: npm link - - name: Install only production dependencies run: npm ci --production --ignore-scripts + - name: Link + run: npm link --ignore-scripts + - name: Link and test working-directory: test-types run: | - npm link @primer/react + npm link @primer/react --ignore-scripts npm tsc --noEmit From 2affdfd6633af327f8efd10019560e67ba614472 Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 11:46:47 -0500 Subject: [PATCH 05/16] Add changeset --- .changeset/khaki-colts-run.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/khaki-colts-run.md diff --git a/.changeset/khaki-colts-run.md b/.changeset/khaki-colts-run.md new file mode 100644 index 00000000000..3e919e2d729 --- /dev/null +++ b/.changeset/khaki-colts-run.md @@ -0,0 +1,6 @@ +--- +'@primer/react': patch +--- + +Fix [an issue](https://github.com/primer/react/issues/1849) where transitive +dependency interface augmentations appeared in our build output From 096ca221afa09a5ffb0c8d313113eee992cd2b3b Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 12:10:12 -0500 Subject: [PATCH 06/16] Remove test-types from eslint config --- .eslintrc.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.eslintrc.json b/.eslintrc.json index f70f9c9c60f..8cfb1b1e801 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -22,7 +22,8 @@ "dist/**/*", "lib/**/*", "lib-*/**/*", - "types/**/*" + "types/**/*", + "test-types/**/*" ], "globals": { "__DEV__": "readonly" From 4f528073e67a527491c82295d861e2316f8165be Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 12:11:21 -0500 Subject: [PATCH 07/16] Adjust ignore-scripts --- .github/workflows/test_types.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index 2b8fd54f19f..672a0ec1cc2 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -18,19 +18,19 @@ jobs: node-version: 16 - name: Install dependencies - run: npm ci --ignore-scripts + run: npm ci - name: Build run: npm run build + - name: Link + run: npm link + - name: Install only production dependencies run: npm ci --production --ignore-scripts - - name: Link - run: npm link --ignore-scripts - - name: Link and test working-directory: test-types run: | npm link @primer/react --ignore-scripts - npm tsc --noEmit + npx tsc --noEmit From 5c0f23bd0a158f3952658f557e3b56236c0a5785 Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 12:15:14 -0500 Subject: [PATCH 08/16] Fix newlines --- test-types/App.tsx | 2 +- test-types/package.json | 2 +- test-types/tsconfig.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test-types/App.tsx b/test-types/App.tsx index 5eadc1651da..a34d321edf8 100644 --- a/test-types/App.tsx +++ b/test-types/App.tsx @@ -2,4 +2,4 @@ import {Box} from '@primer/react' export default function App() { return -} \ No newline at end of file +} diff --git a/test-types/package.json b/test-types/package.json index 2a2d7fa13da..a0b152ecd04 100644 --- a/test-types/package.json +++ b/test-types/package.json @@ -2,4 +2,4 @@ "dependencies": { "@primer/react": "*" } -} \ No newline at end of file +} diff --git a/test-types/tsconfig.json b/test-types/tsconfig.json index 276f18a6351..1a898abb1a0 100644 --- a/test-types/tsconfig.json +++ b/test-types/tsconfig.json @@ -11,7 +11,7 @@ "strict": true, "moduleResolution": "node", "esModuleInterop": true, - "forceConsistentCasingInFileNames": true, + "forceConsistentCasingInFileNames": true }, "include": ["./*.tsx"] } From dbd45df79bb5b4dac4e0db71832a08645249c5ed Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 12:39:53 -0500 Subject: [PATCH 09/16] Remove prepare from package.json in test-types workflow --- .github/workflows/test_types.yml | 13 +++++++++---- package.json | 3 +-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index 672a0ec1cc2..0217afe0afb 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -17,20 +17,25 @@ jobs: with: node-version: 16 + - name: Remove "prepare" script + run: | + jq 'del(.scripts.prepare)' < package.json > package-no-prepare.json + mv package-no-prepare.json package.json + - name: Install dependencies run: npm ci - name: Build run: npm run build + - name: Install only production dependencies + run: npm ci --production + - name: Link run: npm link - - name: Install only production dependencies - run: npm ci --production --ignore-scripts - - name: Link and test working-directory: test-types run: | - npm link @primer/react --ignore-scripts + npm link @primer/react npx tsc --noEmit diff --git a/package.json b/package.json index 91fe80d65f8..53b977855d1 100644 --- a/package.json +++ b/package.json @@ -38,8 +38,7 @@ "test": "jest", "test:update": "npm run test -- --updateSnapshot", "release": "npm run build && changeset publish", - "size": "size-limit", - "prepare": "husky install" + "size": "size-limit" }, "repository": "primer/react", "keywords": [ From a3c249888155b488001be7701a5cf1e130112a32 Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 12:40:41 -0500 Subject: [PATCH 10/16] Add comment about package.json/prepare --- .github/workflows/test_types.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index 0217afe0afb..541650db6f1 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -17,6 +17,10 @@ jobs: with: node-version: 16 + # `prepare` is a special case in `npm` and likes to run all the time, even + # with `--ignore-scripts` and even when using `npm link @primer/react + # --ignore-scripts`. This just removes it entirely for the duration of + # this workflow. - name: Remove "prepare" script run: | jq 'del(.scripts.prepare)' < package.json > package-no-prepare.json From 63e88fad48c640b9a34736a8aecca5daad20f61a Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 12:43:53 -0500 Subject: [PATCH 11/16] Add npm install to test types --- .github/workflows/test_types.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index 541650db6f1..8894f3dad2e 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -41,5 +41,6 @@ jobs: - name: Link and test working-directory: test-types run: | + npm install npm link @primer/react npx tsc --noEmit From 9953a28bed5f95f4d9959ce15c426e26b487588f Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 12:50:00 -0500 Subject: [PATCH 12/16] Add npm run check script --- .github/workflows/test_types.yml | 2 +- test-types/package.json | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index 8894f3dad2e..83969b6d6b2 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -43,4 +43,4 @@ jobs: run: | npm install npm link @primer/react - npx tsc --noEmit + npm run check diff --git a/test-types/package.json b/test-types/package.json index a0b152ecd04..a533918e52a 100644 --- a/test-types/package.json +++ b/test-types/package.json @@ -1,5 +1,9 @@ { + "scripts": { + "check": "tsc --noEmit" + }, "dependencies": { - "@primer/react": "*" + "@primer/react": "*", + "typescript": "^4.4.4" } } From e0be7e263457ed46f41832e8e1d23f7e35a993b4 Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 13:01:16 -0500 Subject: [PATCH 13/16] Clean up prepare removal step --- .github/workflows/test_types.yml | 4 +--- package.json | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index 83969b6d6b2..a5920c92a20 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -22,9 +22,7 @@ jobs: # --ignore-scripts`. This just removes it entirely for the duration of # this workflow. - name: Remove "prepare" script - run: | - jq 'del(.scripts.prepare)' < package.json > package-no-prepare.json - mv package-no-prepare.json package.json + run: echo $(jq 'del(.scripts.prepare)' < package.json) > package.json - name: Install dependencies run: npm ci diff --git a/package.json b/package.json index 53b977855d1..91fe80d65f8 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,8 @@ "test": "jest", "test:update": "npm run test -- --updateSnapshot", "release": "npm run build && changeset publish", - "size": "size-limit" + "size": "size-limit", + "prepare": "husky install" }, "repository": "primer/react", "keywords": [ From 9d224d87f3ee17caa02579f9d407e018b98cfb98 Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 13:03:43 -0500 Subject: [PATCH 14/16] Use npm pkg to remove prepare script --- .github/workflows/test_types.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index a5920c92a20..58e8f585a46 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -22,7 +22,7 @@ jobs: # --ignore-scripts`. This just removes it entirely for the duration of # this workflow. - name: Remove "prepare" script - run: echo $(jq 'del(.scripts.prepare)' < package.json) > package.json + run: npm pkg delete scripts.prepare - name: Install dependencies run: npm ci From bbca53f5d03b0e7c541b74d8bb3782e9b1b8dbd6 Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 13:27:02 -0500 Subject: [PATCH 15/16] Add npm cache to test_types workflow --- .github/workflows/test_types.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test_types.yml b/.github/workflows/test_types.yml index 58e8f585a46..d53a05e69e0 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/test_types.yml @@ -16,6 +16,7 @@ jobs: uses: actions/setup-node@v2 with: node-version: 16 + cache: npm # `prepare` is a special case in `npm` and likes to run all the time, even # with `--ignore-scripts` and even when using `npm link @primer/react From a09d63ddf6d7af00375b09027559bce88019c4ce Mon Sep 17 00:00:00 2001 From: Jonathan Clem Date: Thu, 10 Feb 2022 14:59:35 -0500 Subject: [PATCH 16/16] Add annotations when consumer test fails (#1857) * Add annotation step to test_types workflow * Remove branch restriction from `pull_request` event * Make tsconfig.build.json fail consumer test * Add always() to if clause in test_types workflow * Use clearer failed() instead of always() * Remove brackets from workflow if * s/status/conclusion * s/failed/failure * Add longer annotation * Format workflow echo * Attempt to use cat for multi-line annotation * Rename to consumer test * Add issue context links * Nicer links in consumer test readme * Revert "Make tsconfig.build.json fail consumer test" This reverts commit f6a367872729523c5ebe65e8196a51b10130cbb5. * Add consumer-test to eslintrc ignore --- .eslintrc.json | 2 +- .../{test_types.yml => consumer_test.yml} | 16 ++++++++---- {test-types => consumer-test}/App.tsx | 0 consumer-test/README.md | 25 +++++++++++++++++++ {test-types => consumer-test}/package.json | 0 {test-types => consumer-test}/tsconfig.json | 0 6 files changed, 37 insertions(+), 6 deletions(-) rename .github/workflows/{test_types.yml => consumer_test.yml} (71%) rename {test-types => consumer-test}/App.tsx (100%) create mode 100644 consumer-test/README.md rename {test-types => consumer-test}/package.json (100%) rename {test-types => consumer-test}/tsconfig.json (100%) diff --git a/.eslintrc.json b/.eslintrc.json index 8cfb1b1e801..c521cee7d2c 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -23,7 +23,7 @@ "lib/**/*", "lib-*/**/*", "types/**/*", - "test-types/**/*" + "consumer-test/**/*" ], "globals": { "__DEV__": "readonly" diff --git a/.github/workflows/test_types.yml b/.github/workflows/consumer_test.yml similarity index 71% rename from .github/workflows/test_types.yml rename to .github/workflows/consumer_test.yml index d53a05e69e0..659b0a7addf 100644 --- a/.github/workflows/test_types.yml +++ b/.github/workflows/consumer_test.yml @@ -1,12 +1,12 @@ -name: Test types +name: Consumer test on: push: {branches: main} - pull_request: {branches: main} + pull_request: workflow_dispatch: jobs: - test-types: - name: Test types + consumer-test: + name: Consumer test runs-on: ubuntu-latest steps: - name: Checkout repository @@ -38,8 +38,14 @@ jobs: run: npm link - name: Link and test - working-directory: test-types + id: link-and-test + working-directory: consumer-test run: | npm install npm link @primer/react npm run check + + - name: Add annotation + if: failure() && steps.link-and-test.conclusion == 'failure' + run: | + echo "::error file=tsconfig.build.json::Test package could not build. See https://github.com/primer/react/blob/main/consumer-test" diff --git a/test-types/App.tsx b/consumer-test/App.tsx similarity index 100% rename from test-types/App.tsx rename to consumer-test/App.tsx diff --git a/consumer-test/README.md b/consumer-test/README.md new file mode 100644 index 00000000000..6e7ac65e3ba --- /dev/null +++ b/consumer-test/README.md @@ -0,0 +1,25 @@ +# Primer React Consumer Test + +This directory is used to run a simple test that asserts that a consumer of +Primer React can build their own project with strict TypeScript options enabled, +including `"skipLibCheck": false`. + +During Primer React's build process, we run the TypeScript compiler and output +`.d.ts` declaration files for consumers of Primer React that are using +TypeScript. If the build script runs with a TypeScript configuration that has +any files in its `types` or `typeRoots` that import any of our development +dependencies, it's possible for our build output to be polluted by interface +augmentations in those dependencies, or in transitive dependencies. + +The best way to avoid this is to ensure that any files that import development +dependencies are excluded in our `tsconfig.build.json` file we use to build +Primer React. + +If a mistake is made and a file is omitted, we will catch those when we attempt +to build this consumer library, which has `"skipLibCheck": false` in its +TypeScript configuration. + +For historical context, see these issues: + +- [v27.0.0 breaks TypeScript typings](https://github.com/primer/react/issues/1163) +- [Storybook dependency changes types in build output](https://github.com/primer/react/issues/1849) diff --git a/test-types/package.json b/consumer-test/package.json similarity index 100% rename from test-types/package.json rename to consumer-test/package.json diff --git a/test-types/tsconfig.json b/consumer-test/tsconfig.json similarity index 100% rename from test-types/tsconfig.json rename to consumer-test/tsconfig.json