#736 Fix typescript error with private variable

[subzero10]

Status

READY

Description

Fixes #736.

We are generating two sets of .d.ts files. It appears that when using the intersection type (typeof Server & typeof Browser) to export a Honeybadger typed singleton that supports both contexts, Typescript is complaining about the private variables because they come from different classes. These classes are the same but they come from different folders (dist/browser/core/client.d.ts and dist/server/core/client.d.ts). I think Typescript can't see that the variable is the same for both types, and assumes each type has their own unique private variable.

The easy solution without going too deep was to make this variable as protected, which is OK. The extending classes could use that variable (they don't use it now).

I feel that there may be a better way to generate types (and ultimately we wouldn't have to deal with issues like the above), but the fact that we have one variable to export with different types based on the context (nodejs vs browser) complicates things.

• Change private to protected
• Add test file for Honeybadger.d.ts

[shalvah]

Fair. The intersection thing was introduced by me in #612, because prior to that we were exporting a single type that contained both server and browser fields, which is misleading.

Actually, I don't expect folks to be using the honeybadger.d.ts (but I don't really know about other usages outside Node.js). If you require() the server.js (which is also the main file in package.json), you'll get the server.d.ts types. Dunno about browser and other usage.

[subzero10]

Fair. The intersection thing was introduced by me in #612, because prior to that we were exporting a single type that contained both server and browser fields, which is misleading.

Actually, I don't expect folks to be using the honeybadger.d.ts (but I don't really know about other usages outside Node.js). If you require() the server.js (which is also the main file in package.json), you'll get the server.d.ts types. Dunno about browser and other usage.

Hmm, I'm not sure I follow. If you do import Honeybadger from '@honeybadger-io/js', it will import the honeybadger.d.ts file. Probably because we have set "types": "./honeybadger.d.ts" in package.json.
How would you require the server.js?

[shalvah]

1. You can do @honeybadger-io/js/server
2. Even if you don't, PHPStorm finds the correct type (Server from server/honeybadger.d.ts), probably because main is server/honeybadger.js and that typedef is located next to it.

Of course, disclaimers:

1. This works for me, in a vanilla (non-TypeScript) project, ierequire("@honeybadger-io/js"), I get server-only types.
2. I think this has also worked in a TS project for me, but I can't remember. I should mention that I still use require in my TS.
3. I prefer not to mention import, because TypeScript's import (actually CommonJS) and ES6 imports (.mjs) are two different things, and that is a whole other complicated issue on its own.
4. I have no idea what you get when writing code for the browser (or even how to test that; I don't work with frontend tooling).

Thing is, we're generally fucked, thanks to the universal package thing. Before that PR, the type inference was pretty poor. The exported type defs were written manually, leading to incomplete information. Plus, a single type for both server and browser meant autocomplete would show you options that don't actually exist in your build. That PR fixed most of those. There's still a bit of fuckery, but the advantage is that a user that needs more precise type definitions can require @honeybadger-io/js/server or @honeybadger-io/js/browser instead (which gave the same typedefs in the past).

[subzero10]

1. You can do @honeybadger-io/js/server

Ah OK, though I guess this is not the recommended use.

2. Even if you don't, PHPStorm finds the correct type (Server from server/honeybadger.d.ts), probably because main is server/honeybadger.js and that typedef is located next to it.

Hmm, I'm using Intellij (similar to PHPStorm) and it goes to the main honeybadger.d.ts 🤷 . I just tried with a Typescript project.

Anyway, thank you for the detailed explanation! I will proceed merging so that the typings will be fixed and I will put a note to revisit this at some point (or at least test thoroughly to be certain on how typescript behaves on nodejs/browser with our current approach).

[shalvah]

Ah OK, though I guess this is not the recommended use.

Yeah, but methinks maybe it should be? There aren't any downsides, while the other approach has obvious downsides. Honestly, telling people to require /server or /browser would eliminate these type-related issues.

[subzero10]

True, though I feel that if we go down that path, we should just publish different packages.
Actually, that's why I started the discussion on the monorepo. This repo is growing and becoming complex, sometimes because of the different environements/platforms/frameworks we want to support. I feel that maybe we should come up with smaller, more isolated packages and having a monorepo would help with the context switching.

[shalvah]

Agree! But there were two packages in the past (https://docs.honeybadger.io/lib/javascript/support/upgrading-to-v3/), which were then consolidated into one. So going back to 2 may not come off well.

[subzero10]

Hmm, do you know why? I'm thinking because it was mostly same/shared code.

[subzero10]

@shipjs prepare

[github-actions]

@subzero10 shipjs prepare fail

[github-actions]

@subzero10 shipjs prepare fail

[shalvah]

Yes, I think so. The mistake was in exposing the combined repo as a single package rather than as separate packages. 😓 Server and browser environments are significantly different; trying to treat them identically just leads to a subpar experience for both (for instance, the unnecessary transpilation and bundling we're doing for the server code).

[subzero10]

OK thanks, let's resume this discussion when @joshuap is back from vacation and consider this in our exploration for the monorepo.

[shalvah]

But perhaps we could still do it again. Everyone makes mistakes. It would be annoying, but upgrading should be as easy as replacing /js with /server or /browser.

[joshuap]

Yes, I think so. The mistake was in exposing the combined repo as a single package rather than as separate packages. 😓 Server and browser environments are significantly different; trying to treat them identically just leads to a subpar experience for both (for instance, the unnecessary transpilation and bundling we're doing for the server code).

There are significant benefits to the universal package (especially for front-end frameworks that do SSR). It was a huge PITA to maintain two totally separate packages (with different APIs). The way it's set up now, it should be straightforward to release server and browser packages in addition to the current universal package; that was my intention all along, but I decided to start with just the universal and wait to see if/when it would be helpful to also have additional packages for each runtime. I'm not as familiar with the TypeScript issues, but I know that the "main" and "browser" directives in package.json can be tricky because not all bundlers use them the same way (like Webpack does, for instance). I assume that's more of an issue with bundlers not properly supporting the convention.

When I originally looked into using a monorepo, my plan was to have four packages:

• @honeybadger-io/core
• @honeybadger-io/browser
• @honeybadger-io/server
• @honeybadger-io/js (isomorphic)

If we move to a monorepo now, I still think this would be a good structure (and be a totally transparent change to end-users of the current @honeybadger-io/js package).

Edit: Btw, I chose not to go with the monorepo earlier because I had similar concerns to @shalvah about tooling complexity. The tradeoff could be worth it now—but it's definitely a tradeoff. Being able to refactor the current project into multiple packages (as describe above) would be the biggest reason to accept the extra complexity, in my opinion.