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

Remove transitive dependency type augmentations from build output #1855

Merged
merged 17 commits into from
Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/khaki-colts-run.md
Original file line number Diff line number Diff line change
@@ -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
3 changes: 2 additions & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"dist/**/*",
"lib/**/*",
"lib-*/**/*",
"types/**/*"
"types/**/*",
"consumer-test/**/*"
],
"globals": {
"__DEV__": "readonly"
Expand Down
51 changes: 51 additions & 0 deletions .github/workflows/consumer_test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
name: Consumer test
on:
push: {branches: main}
pull_request:
workflow_dispatch:

jobs:
consumer-test:
name: Consumer test
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

# `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: npm pkg delete scripts.prepare

- 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: Link and test
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"
5 changes: 5 additions & 0 deletions consumer-test/App.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import {Box} from '@primer/react'

export default function App() {
return <Box />
}
25 changes: 25 additions & 0 deletions consumer-test/README.md
Original file line number Diff line number Diff line change
@@ -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)
9 changes: 9 additions & 0 deletions consumer-test/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"scripts": {
"check": "tsc --noEmit"
},
"dependencies": {
"@primer/react": "*",
"typescript": "^4.4.4"
}
}
17 changes: 17 additions & 0 deletions consumer-test/tsconfig.json
Original file line number Diff line number Diff line change
@@ -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"]
}
13 changes: 9 additions & 4 deletions tsconfig.build.json
Original file line number Diff line number Diff line change
@@ -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"]
}