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

ci: Fix build not including typescript types #2076

Merged
merged 3 commits into from
May 12, 2022
Merged

ci: Fix build not including typescript types #2076

merged 3 commits into from
May 12, 2022

Conversation

brandonlenz
Copy link
Contributor

@brandonlenz brandonlenz commented May 9, 2022

Summary

Looks like when the TS version was bumped, the refactor to remove awesome-typescript-loader and rely purely on babel didn't backfilll for type generation which babel does not do on its own. In this PR we manually call tsc in the build step and let it emit the type declarations. This is the recommended approach for projects using babel alone and not relying on ts-loader.

Alternative considered: use ts loader on ts|x files before running babel. ts-loader is a bit slow, and felt like a more repetitive solution, using two loaders on all of our ts/tsx files when tsc will directly create just the type exports for us without any additional config.

Resources:

Related Issues or PRs

closes: #2053
stems from: #1548

How To Test

Checkout the code, clean your local files (delete node_modules and lib), freshly install packages with yarn. running yarn will also run yarn build which will create the lib directory where you can confirm that .d.ts files are now output correctly again. Running yarn pack will generate the tarball which you can also verify includes the types.

> git checkout bl-fix-types
> rm -rf node_modules
> rm -rf lib
> yarn
> yarn pack

Screenshots (optional):

image

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2022

Warnings
⚠️

Changes were made to package.json, but not to yarn.lock - Perhaps you need to run yarn install?

⚠️ This PR does not include changes to storybook, even though it affects component code.
⚠️ This PR does not include changes to tests, even though it affects source code.

Generated by 🚫 dangerJS against 63ccd1c

export { ModalToggleButton } from './components/Modal/ModalToggleButton'
export { ModalOpenLink } from './components/Modal/ModalOpenLink'
export { ModalHeading } from './components/Modal/ModalHeading/ModalHeading'
export { ModalFooter } from './components/Modal/ModalFooter/ModalFooter'
export type { ModalProps, ModalRef } from './components/Modal/Modal'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These export type changes silence some TS warnings we were getting on build

@@ -11,11 +11,12 @@
"module": "esnext",
"moduleResolution": "node",
"resolveJsonModule": true,
"noEmit": true,
"emitDeclarationOnly": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, awesome-typescript-loader was exporting the types. Now we rely on tsc to do it, so we emit those type declarations only, and let babel emit the rest of the transpiled source code

"jsx": "react",
"noImplicitAny": true,
"declaration": true,
"outDir": "./lib"
"outDir": "./lib",
"isolatedModules": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +30 to +31
"build": "tsc && webpack --progress",
"build:watch": "tsc && webpack --watch",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manually call tsc here so that types are built to lib

"lint": "tsc --noEmit && eslint --ext js,jsx,ts,tsx src && stylelint \"src/**/*.{css,scss}\"",
"build": "tsc && webpack --progress",
"build:watch": "tsc && webpack --watch",
"lint": "tsc --noEmit --emitDeclarationOnly false && eslint --ext js,jsx,ts,tsx src && stylelint \"src/**/*.{css,scss}\"",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

noEmit cannot be used alongside emitDeclarationOnly (as of ts 3.8), which is now in tsconfig. So we have to set it to false here, which is the recommended workaround

Copy link
Contributor

@hgarfinkle hgarfinkle left a comment

Choose a reason for hiding this comment

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

This tentatively looks good to me... how can we check that this fixes the problem?

@brandonlenz
Copy link
Contributor Author

This tentatively looks good to me... how can we check that this fixes the problem?

The How to test steps involve running the commands that our release CI runs to package the library. Following those steps results in properly packaged type definitions.

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.

[fix] react-uswds no longer ships a .d.ts file, cannot be used with TypeScript any more
3 participants