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

Improve TypeScript usage #612

Merged
merged 10 commits into from
Jul 19, 2021
Merged

Improve TypeScript usage #612

merged 10 commits into from
Jul 19, 2021

Conversation

shalvah
Copy link
Contributor

@shalvah shalvah commented Jul 16, 2021

(Apologies for the size of the PR; the changes involved turned out to be a little more than anticipated.)

This PR:

  1. Adds @types/express to our dependencies, fixing Typescript module error #604
  2. Adds some missing null checks. A number of places had a check for if (typeof thing === 'object'), with the subtle bug that null in JS also returns object.
  3. Improves typing. A number of types (some of which were user-facing) were incorrect or incomplete. I've fixed them so they're (mostly) correct, providing better IDE suggestions.
  4. Switches to (mostly) automated generation of type declarations. This is done by enabling declarations in our tsconfig.json, then updating our Rollup config to point to the right paths for each build. However, this generates separate type decl files for the two builds, so there's a manual honeybadger.d.ts file that re-exports them as one file as before. Additionally, users can still import the browser/honeybadger.d.ts or server/honeybadger.d.ts files like before (with the added advantage of accurate types for the build).
  5. Updated the README to explain this for first-time contributors.

@shalvah
Copy link
Contributor Author

shalvah commented Jul 16, 2021

Also: while working on this, I noticed there's nothing in the docs about working with TypeScript or ES Modules. For instance, should users do import Honeybadger from '...' or import * as Honeybadger from '...'. It's something that would be worth addressing eventually.

Not a priority (React doesn't even support native ESM yet) but I think some folks will eventually start asking about it as ESM gains traction.

@shalvah shalvah changed the title Rework TypeScript usage Improve TypeScript usage Jul 16, 2021
@joshuap joshuap self-assigned this Jul 19, 2021
Copy link
Member

@joshuap joshuap left a comment

Choose a reason for hiding this comment

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

This is beautiful. Thanks!

@joshuap joshuap merged commit 7d49aab into master Jul 19, 2021
@joshuap
Copy link
Member

joshuap commented Jul 19, 2021

@shipjs prepare

@github-actions
Copy link
Contributor

@joshuap shipjs prepare fail

@shalvah shalvah deleted the rework-typescript branch July 19, 2021 17:44
@shalvah
Copy link
Contributor Author

shalvah commented Jul 20, 2021

👍

image

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